Re: cygwin 3.3.x: another problem that may be related to pipes

[Ken Brown <kbrown@cornell.edu>]

On 11/15/2021 10:11 AM, Takashi Yano wrote:
> On Mon, 15 Nov 2021 23:36:01 +0900
> Takashi Yano wrote:
>> On Mon, 15 Nov 2021 08:57:43 -0500
>> Ken Brown wrote:
>>> On 11/15/2021 8:01 AM, Corinna Vinschen wrote:
>>>> [Redirected to cygwin-developers]
>>>>
>>>> On Nov 15 17:18, Takashi Yano via Cygwin wrote:
>>>>> raw_read()/raw_write() in fhandler_pipe.cc seems to need handling
>>>>> of STATUS_PENDING in nonblocking mode.
>>>>>
>>>>> I also confirmed the following patch fixes the issue. However, I
>>>>> am not very sure that this is the right thing.
>>>>>
>>>>> Corinna, Ken, what do you think?
>>>>
>>>> I'm puzzled.  non-blocking pipes return STATUS_PENDING?  What on earth
>>>> is that supposed to mean on non-blocking (as opposed to asynchronous)
>>>> pipes?  The problem is that STATUS_PENDING theoretically requires
>>>> to wait for... something, the pipe handle, perhaps, and then to check
>>>> io.Status.
>>>
>>> I noticed last week when I was debugging the XEmacs problem that NtReadFile
>>> occasionally returned STATUS_PENDING.  I commented on it here:
>>>
>>>     https://cygwin.com/pipermail/cygwin/2021-November/249827.html
>>>
>>> But I promptly forgot about it when that turned out not to be the problem for
>>> that bug.  My thought at the time was that we needed something like this:
>>>
>>> diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
>>> index 9392a28c1..aaf09829d 100644
>>> --- a/winsup/cygwin/fhandler_pipe.cc
>>> +++ b/winsup/cygwin/fhandler_pipe.cc
>>> @@ -336,9 +336,10 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
>>>           break;
>>>          status = NtReadFile (get_handle (), evt, NULL, NULL, &io, ptr,
>>>                              len1, NULL, NULL);
>>> -      if (evt && status == STATUS_PENDING)
>>> +      if (status == STATUS_PENDING)
>>>           {
>>> -         waitret = cygwait (evt, INFINITE, cw_cancel | cw_sig);
>>> +         /* Very short-lived in the non-blocking case. */
>>> +         waitret = cygwait (evt ?: get_handle (), INFINITE, cw_cancel | cw_sig);
>>>             /* If io.Status is STATUS_CANCELLED after CancelIo, IO has actually
>>>                been cancelled and io.Information contains the number of bytes
>>>                processed so far.
>>>
>>> Takashi, does that fix the problem?
>>
>> IIUC, WaitForMultipleObject() cannot wait for pipe object.
>> Only the following objects are allowed to be waited.
>>
>> Change notification
>> Console input
>> Event
>> Memory resource notification
>> Mutex
>> Process
>> Semaphore
>> Thread
>> Waitable timer
>>
>> Note that the problem reported is not in raw_read(), but in raw_write().
>>
>> If you mean applying above patch for raw_write(), it proberbly fixes the
>> issue as well.
>>
>> This is because ...
>>
>> If you pass the pipe handle to WFMO, it imediately returns WAIT_OBJECT_0,
>> so your patch will work almost same with my patch.
> 
> I might be wrong. Your code certainly waits for something happening.
> I am not sure why it works...

FWIW, I got the idea for this from the following code in 
fhandler_socket_unix::listen_pipe:

   io.Status = STATUS_PENDING;
   if (!is_nonblocking () && !(evt = create_event ()))
     return -1;
   status = NtFsControlFile (get_handle (), evt, NULL, NULL, &io,
			    FSCTL_PIPE_LISTEN, NULL, 0, NULL, 0);
   if (status == STATUS_PENDING)
     {
       waitret = cygwait (evt ?: get_handle (), cw_infinite,
			 cw_cancel | cw_sig_eintr);
       if (waitret == WAIT_OBJECT_0)
	status = io.Status;
     }

Ken