public inbox for cygwin-developers@cygwin.com
 help / color / mirror / Atom feed
* Re: cygwin 3.3.x: another problem that may be related to pipes
       [not found]   ` <20211115171811.844dce9cce2b4d13262d64f2@nifty.ne.jp>
@ 2021-11-15 13:01     ` Corinna Vinschen
  2021-11-15 13:57       ` Ken Brown
  2021-11-15 14:50       ` Takashi Yano
  0 siblings, 2 replies; 20+ messages in thread
From: Corinna Vinschen @ 2021-11-15 13:01 UTC (permalink / raw)
  To: cygwin-developers

[Redirected to cygwin-developers]

On Nov 15 17:18, Takashi Yano via Cygwin wrote:
> On Sun, 14 Nov 2021 20:50:59 +0000 (UTC)
> bf wrote:
> > I've a shell script that processes several hundred xz-compressed text files with a pipeline of the form:
> > 
> > find . -depth -type f  ... -print0 | \
> > xargs -0tI @ xzcat -- @ | \
> > iconv --byte-subst='�' --unicode-subst='�' --widechar-subst='�' -f MS-ANSI -t UTF-8 -- | \
> > grep ... \
> > sed ... | \
> > sort ... | \
> > sort ...
> > 
> > . Since updating to cygwin 3.3.x (but not on cygwin 3.2.x) the script consistently fails when decompressing a portion of the files, resulting in lost data, and error messages of the form:
> > 
> > xzcat: (stdout): Write error: Bad address
> 
> Thanks for the report.
> I could reproduce your problem and found the cause.
> 
> 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.

Is it possible that nonblocking pipes need to be synchronous, i. e.,
setting the FILE_SYNCHRONOUS_IO_NONALERT would fix that?


Thanks,
Corinna

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: cygwin 3.3.x: another problem that may be related to pipes
  2021-11-15 13:01     ` cygwin 3.3.x: another problem that may be related to pipes Corinna Vinschen
@ 2021-11-15 13:57       ` Ken Brown
  2021-11-15 14:36         ` Takashi Yano
  2021-11-15 14:50       ` Takashi Yano
  1 sibling, 1 reply; 20+ messages in thread
From: Ken Brown @ 2021-11-15 13:57 UTC (permalink / raw)
  To: cygwin-developers

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?

> Is it possible that nonblocking pipes need to be synchronous, i. e.,
> setting the FILE_SYNCHRONOUS_IO_NONALERT would fix that?

Ken

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: cygwin 3.3.x: another problem that may be related to pipes
  2021-11-15 13:57       ` Ken Brown
@ 2021-11-15 14:36         ` Takashi Yano
  2021-11-15 15:11           ` Takashi Yano
  2021-11-15 16:25           ` Corinna Vinschen
  0 siblings, 2 replies; 20+ messages in thread
From: Takashi Yano @ 2021-11-15 14:36 UTC (permalink / raw)
  To: cygwin-developers

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.


-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: cygwin 3.3.x: another problem that may be related to pipes
  2021-11-15 13:01     ` cygwin 3.3.x: another problem that may be related to pipes Corinna Vinschen
  2021-11-15 13:57       ` Ken Brown
@ 2021-11-15 14:50       ` Takashi Yano
  2021-11-15 16:08         ` Corinna Vinschen
  1 sibling, 1 reply; 20+ messages in thread
From: Takashi Yano @ 2021-11-15 14:50 UTC (permalink / raw)
  To: cygwin-developers

On Mon, 15 Nov 2021 14:01:12 +0100
Corinna Vinschen wrote:
> [Redirected to cygwin-developers]
> 
> On Nov 15 17:18, Takashi Yano via Cygwin wrote:
> > On Sun, 14 Nov 2021 20:50:59 +0000 (UTC)
> > bf wrote:
> > > I've a shell script that processes several hundred xz-compressed text files with a pipeline of the form:
> > > 
> > > find . -depth -type f  ... -print0 | \
> > > xargs -0tI @ xzcat -- @ | \
> > > iconv --byte-subst='�' --unicode-subst='�' --widechar-subst='�' -f MS-ANSI -t UTF-8 -- | \
> > > grep ... \
> > > sed ... | \
> > > sort ... | \
> > > sort ...
> > > 
> > > . Since updating to cygwin 3.3.x (but not on cygwin 3.2.x) the script consistently fails when decompressing a portion of the files, resulting in lost data, and error messages of the form:
> > > 
> > > xzcat: (stdout): Write error: Bad address
> > 
> > Thanks for the report.
> > I could reproduce your problem and found the cause.
> > 
> > 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.
> 
> Is it possible that nonblocking pipes need to be synchronous, i. e.,
> setting the FILE_SYNCHRONOUS_IO_NONALERT would fix that?

Yes. Setting FILE_SYNCHRONOUS_IO_NONALERT for the write pipe seems
to fix the issue. Proberbly, this needs the modification for
raw_write() like the modification I have made for raw_read() in
topic/pipe branch.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: cygwin 3.3.x: another problem that may be related to pipes
  2021-11-15 14:36         ` Takashi Yano
@ 2021-11-15 15:11           ` Takashi Yano
  2021-11-15 15:27             ` Ken Brown
  2021-11-15 16:25           ` Corinna Vinschen
  1 sibling, 1 reply; 20+ messages in thread
From: Takashi Yano @ 2021-11-15 15:11 UTC (permalink / raw)
  To: cygwin-developers

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...

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: cygwin 3.3.x: another problem that may be related to pipes
  2021-11-15 15:11           ` Takashi Yano
@ 2021-11-15 15:27             ` Ken Brown
  2021-11-15 15:36               ` Ken Brown
  0 siblings, 1 reply; 20+ messages in thread
From: Ken Brown @ 2021-11-15 15:27 UTC (permalink / raw)
  To: cygwin-developers

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: cygwin 3.3.x: another problem that may be related to pipes
  2021-11-15 15:27             ` Ken Brown
@ 2021-11-15 15:36               ` Ken Brown
  2021-11-15 16:45                 ` Takashi Yano
  0 siblings, 1 reply; 20+ messages in thread
From: Ken Brown @ 2021-11-15 15:36 UTC (permalink / raw)
  To: cygwin-developers

On 11/15/2021 10:27 AM, Ken Brown wrote:
> On 11/15/2021 10:11 AM, Takashi Yano wrote:
>> 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;
>      }

And here's Corinna's answer when I asked her to explain it:

   https://cygwin.com/pipermail/cygwin-developers/2021-April/012093.html

Ken

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: cygwin 3.3.x: another problem that may be related to pipes
  2021-11-15 14:50       ` Takashi Yano
@ 2021-11-15 16:08         ` Corinna Vinschen
  2021-11-15 16:37           ` Ken Brown
  0 siblings, 1 reply; 20+ messages in thread
From: Corinna Vinschen @ 2021-11-15 16:08 UTC (permalink / raw)
  To: cygwin-developers

On Nov 15 23:50, Takashi Yano wrote:
> On Mon, 15 Nov 2021 14:01:12 +0100
> Corinna Vinschen wrote:
> > [Redirected to cygwin-developers]
> > 
> > On Nov 15 17:18, Takashi Yano via Cygwin wrote:
> > > On Sun, 14 Nov 2021 20:50:59 +0000 (UTC)
> > > bf wrote:
> > > > I've a shell script that processes several hundred xz-compressed text files with a pipeline of the form:
> > > > 
> > > > find . -depth -type f  ... -print0 | \
> > > > xargs -0tI @ xzcat -- @ | \
> > > > iconv --byte-subst='�' --unicode-subst='�' --widechar-subst='�' -f MS-ANSI -t UTF-8 -- | \
> > > > grep ... \
> > > > sed ... | \
> > > > sort ... | \
> > > > sort ...
> > > > 
> > > > . Since updating to cygwin 3.3.x (but not on cygwin 3.2.x) the script consistently fails when decompressing a portion of the files, resulting in lost data, and error messages of the form:
> > > > 
> > > > xzcat: (stdout): Write error: Bad address
> > > 
> > > Thanks for the report.
> > > I could reproduce your problem and found the cause.
> > > 
> > > 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.
> > 
> > Is it possible that nonblocking pipes need to be synchronous, i. e.,
> > setting the FILE_SYNCHRONOUS_IO_NONALERT would fix that?
> 
> Yes. Setting FILE_SYNCHRONOUS_IO_NONALERT for the write pipe seems
> to fix the issue.

That was just a hunch but it doesn't *actually* surprise me...

> Proberbly, this needs the modification for
> raw_write() like the modification I have made for raw_read() in
> topic/pipe branch.

The idea was to do change to synchronous pipes only for 3.4.  I have a
bit of headaches to change the pipes to synchronous as bug fix on the
3.3 branch.  Your first ideas might be better there...

...unless, of course, you and Ken are convinced that this is the best
approach even as 3.3 bugfix.  In that case, just say so and push the
stuff to the 3.3 branch, too, as you see fit.


Thanks,
Corinna

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: cygwin 3.3.x: another problem that may be related to pipes
  2021-11-15 14:36         ` Takashi Yano
  2021-11-15 15:11           ` Takashi Yano
@ 2021-11-15 16:25           ` Corinna Vinschen
  1 sibling, 0 replies; 20+ messages in thread
From: Corinna Vinschen @ 2021-11-15 16:25 UTC (permalink / raw)
  To: cygwin-developers

On Nov 15 23:36, Takashi Yano wrote:
> [...]
> 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

Pipe handles shouldn't be any different than the above.  Please note
that you can also wait, for instance, for socket handles, which are not
in that list either.  It's just that it's tricky and the developer is
potentially doing something which *looks* correct, but doesn't what was
intended at all.

> If you pass the pipe handle to WFMO, it imediately returns WAIT_OBJECT_0,
> so your patch will work almost same with my patch.

If it returns with WAIT_OBJECT_0, something has certainly happened on
the pipe.  The problem is that it's not clear what has happened. If push
comes to shove, it could be a completed action actually performed by
another process.

So here's another brain-dead idea:

What if we create the event even in the nonblocking case?

Considering that the pipe is nonblocking, what *should* happen is this:

- NtReadFile return STATUS_PENDING
- WFMO/cygwait even with INFINITE timeout should return almost immediately
  with WAIT_OBJECT_0
- the io.Status contains a valid status including STATUS_PIPE_EMPTY if
  there's nothing to read

*If* that works, the code in blocking and nonblocking is suddenly
almost identical.  That would be nice, wouldn't it?  If only all
the interesting cases would be well documented...


Corinna

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: cygwin 3.3.x: another problem that may be related to pipes
  2021-11-15 16:08         ` Corinna Vinschen
@ 2021-11-15 16:37           ` Ken Brown
  2021-11-15 16:52             ` Takashi Yano
  0 siblings, 1 reply; 20+ messages in thread
From: Ken Brown @ 2021-11-15 16:37 UTC (permalink / raw)
  To: cygwin-developers

On 11/15/2021 11:08 AM, Corinna Vinschen wrote:
> The idea was to do change to synchronous pipes only for 3.4.  I have a
> bit of headaches to change the pipes to synchronous as bug fix on the
> 3.3 branch.  Your first ideas might be better there...

I agree.  I'd rather fix the bugs with asynchronous pipes on the 3.3 branch and 
only switch to synchronous pipes for 3.4.

Ken

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: cygwin 3.3.x: another problem that may be related to pipes
  2021-11-15 15:36               ` Ken Brown
@ 2021-11-15 16:45                 ` Takashi Yano
  0 siblings, 0 replies; 20+ messages in thread
From: Takashi Yano @ 2021-11-15 16:45 UTC (permalink / raw)
  To: cygwin-developers

On Mon, 15 Nov 2021 10:36:19 -0500
Ken Brown wrote:
> On 11/15/2021 10:27 AM, Ken Brown wrote:
> > On 11/15/2021 10:11 AM, Takashi Yano wrote:
> >> 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;
> >      }
> 
> And here's Corinna's answer when I asked her to explain it:
> 
>    https://cygwin.com/pipermail/cygwin-developers/2021-April/012093.html

I found
https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/nf-ntifs-ntwritefile
says about NtWriteFile():

    If the caller opened the file with the DesiredAccess SYNCHRONIZE
    flag set, the caller can wait for this routine to set the given
    FileHandle to the signaled state.

Also,
https://docs.microsoft.com/en-us/windows/win32/devnotes/ntreadfile
says about NtReadFile():

    If the caller opened the file with the SYNCHRONIZE flag set in
    DesiredAccess, the calling thread can synchronize to the completion
    of the read operation by waiting on the file handle, FileHandle.
    The handle is signaled each time that an I/O operation that was
    issued on the handle completes. However, the caller must not wait
    on a handle that was opened for synchronous file access
    (FILE_SYNCHRONOUS_IO_NONALERT or FILE_SYNCHRONOUS_IO_ALERT).
    In this case, NtReadFile waits on behalf of the caller and does
    not return until the read operation is complete. The caller can
    safely wait on the file handle only if all three of the following
    conditions are met:

      * The handle was opened for asynchronous access (that is, neither
        FILE_SYNCHRONOUS_IO_XXX flag was specified).

      * The handle is being used for only one I/O operation at a time.

      * NtReadFile returned STATUS_PENDING.

Therefore, in this case, waiting for the pipe handle might be
the correct manner.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: cygwin 3.3.x: another problem that may be related to pipes
  2021-11-15 16:37           ` Ken Brown
@ 2021-11-15 16:52             ` Takashi Yano
  2021-11-15 18:47               ` Ken Brown
  0 siblings, 1 reply; 20+ messages in thread
From: Takashi Yano @ 2021-11-15 16:52 UTC (permalink / raw)
  To: cygwin-developers

On Mon, 15 Nov 2021 11:37:30 -0500
Ken Brown wrote:
> On 11/15/2021 11:08 AM, Corinna Vinschen wrote:
> > The idea was to do change to synchronous pipes only for 3.4.  I have a
> > bit of headaches to change the pipes to synchronous as bug fix on the
> > 3.3 branch.  Your first ideas might be better there...
> 
> I agree.  I'd rather fix the bugs with asynchronous pipes on the 3.3 branch and 
> only switch to synchronous pipes for 3.4.

I also agree with you.

So, currently we have three options.

1) Call CancelIo() immediately after STATUS_PENDING like my patch.
2) Wait for pipe handle after STATUS_PENDING for nonblocking mode
   like Ken's patch.
3) Create evt event even for nonblocking mode as Corinna mentioned.

Which is the best solution, do you think?

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: cygwin 3.3.x: another problem that may be related to pipes
  2021-11-15 16:52             ` Takashi Yano
@ 2021-11-15 18:47               ` Ken Brown
  2021-11-15 23:35                 ` Takashi Yano
  0 siblings, 1 reply; 20+ messages in thread
From: Ken Brown @ 2021-11-15 18:47 UTC (permalink / raw)
  To: cygwin-developers

On 11/15/2021 11:52 AM, Takashi Yano wrote:
> On Mon, 15 Nov 2021 11:37:30 -0500
> Ken Brown wrote:
>> On 11/15/2021 11:08 AM, Corinna Vinschen wrote:
>>> The idea was to do change to synchronous pipes only for 3.4.  I have a
>>> bit of headaches to change the pipes to synchronous as bug fix on the
>>> 3.3 branch.  Your first ideas might be better there...
>>
>> I agree.  I'd rather fix the bugs with asynchronous pipes on the 3.3 branch and
>> only switch to synchronous pipes for 3.4.
> 
> I also agree with you.
> 
> So, currently we have three options.
> 
> 1) Call CancelIo() immediately after STATUS_PENDING like my patch.
> 2) Wait for pipe handle after STATUS_PENDING for nonblocking mode
>     like Ken's patch.
> 3) Create evt event even for nonblocking mode as Corinna mentioned.
> 
> Which is the best solution, do you think?

I'm completely unbiased, of course, but I like option 2.

Ken

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: cygwin 3.3.x: another problem that may be related to pipes
  2021-11-15 18:47               ` Ken Brown
@ 2021-11-15 23:35                 ` Takashi Yano
  2021-11-15 23:49                   ` Ken Brown
  0 siblings, 1 reply; 20+ messages in thread
From: Takashi Yano @ 2021-11-15 23:35 UTC (permalink / raw)
  To: cygwin-developers

On Mon, 15 Nov 2021 13:47:44 -0500
Ken Brown wrote:
> On 11/15/2021 11:52 AM, Takashi Yano wrote:
> > On Mon, 15 Nov 2021 11:37:30 -0500
> > Ken Brown wrote:
> >> On 11/15/2021 11:08 AM, Corinna Vinschen wrote:
> >>> The idea was to do change to synchronous pipes only for 3.4.  I have a
> >>> bit of headaches to change the pipes to synchronous as bug fix on the
> >>> 3.3 branch.  Your first ideas might be better there...
> >>
> >> I agree.  I'd rather fix the bugs with asynchronous pipes on the 3.3 branch and
> >> only switch to synchronous pipes for 3.4.
> > 
> > I also agree with you.
> > 
> > So, currently we have three options.
> > 
> > 1) Call CancelIo() immediately after STATUS_PENDING like my patch.
> > 2) Wait for pipe handle after STATUS_PENDING for nonblocking mode
> >     like Ken's patch.
> > 3) Create evt event even for nonblocking mode as Corinna mentioned.
> > 
> > Which is the best solution, do you think?
> 
> I'm completely unbiased, of course, but I like option 2.

OK. Shall I make a patch? Or would you like to do that?
What should we do for master branch? I think topic/pipe
is still under test. So, should we apply the same patch
to master as well as cygwin-3_3-branch?

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: cygwin 3.3.x: another problem that may be related to pipes
  2021-11-15 23:35                 ` Takashi Yano
@ 2021-11-15 23:49                   ` Ken Brown
  2021-11-16  3:28                     ` Takashi Yano
  2021-11-16  9:45                     ` Takashi Yano
  0 siblings, 2 replies; 20+ messages in thread
From: Ken Brown @ 2021-11-15 23:49 UTC (permalink / raw)
  To: cygwin-developers

On 11/15/2021 6:35 PM, Takashi Yano wrote:
> On Mon, 15 Nov 2021 13:47:44 -0500
> Ken Brown wrote:
>> On 11/15/2021 11:52 AM, Takashi Yano wrote:
>>> So, currently we have three options.
>>>
>>> 1) Call CancelIo() immediately after STATUS_PENDING like my patch.
>>> 2) Wait for pipe handle after STATUS_PENDING for nonblocking mode
>>>      like Ken's patch.
>>> 3) Create evt event even for nonblocking mode as Corinna mentioned.
>>>
>>> Which is the best solution, do you think?
>>
>> I'm completely unbiased, of course, but I like option 2.
> 
> OK. Shall I make a patch? Or would you like to do that?

If you don't mind doing it, I'd appreciate it.  I have a lot of Real Life things 
going on at the moment and might not get to it for a couple days.

BTW, I think we have to do the same thing for fhandler_fifo::raw_read, but you 
can leave that for me.

> What should we do for master branch? I think topic/pipe
> is still under test. So, should we apply the same patch
> to master as well as cygwin-3_3-branch?

Yes, I think so.  Then we can rebase topic/pipe onto master.

Thanks.

Ken

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: cygwin 3.3.x: another problem that may be related to pipes
  2021-11-15 23:49                   ` Ken Brown
@ 2021-11-16  3:28                     ` Takashi Yano
  2021-11-16  9:28                       ` Corinna Vinschen
  2021-11-16  9:45                     ` Takashi Yano
  1 sibling, 1 reply; 20+ messages in thread
From: Takashi Yano @ 2021-11-16  3:28 UTC (permalink / raw)
  To: cygwin-developers

On Mon, 15 Nov 2021 18:49:34 -0500
Ken Brown wrote:
> On 11/15/2021 6:35 PM, Takashi Yano wrote:
> > On Mon, 15 Nov 2021 13:47:44 -0500
> > Ken Brown wrote:
> >> On 11/15/2021 11:52 AM, Takashi Yano wrote:
> >>> So, currently we have three options.
> >>>
> >>> 1) Call CancelIo() immediately after STATUS_PENDING like my patch.
> >>> 2) Wait for pipe handle after STATUS_PENDING for nonblocking mode
> >>>      like Ken's patch.
> >>> 3) Create evt event even for nonblocking mode as Corinna mentioned.
> >>>
> >>> Which is the best solution, do you think?
> >>
> >> I'm completely unbiased, of course, but I like option 2.
> > 
> > OK. Shall I make a patch? Or would you like to do that?
> 
> If you don't mind doing it, I'd appreciate it.  I have a lot of Real Life things 
> going on at the moment and might not get to it for a couple days.

OK. I submitted the patch.

> BTW, I think we have to do the same thing for fhandler_fifo::raw_read, but you 
> can leave that for me.
> 
> > What should we do for master branch? I think topic/pipe
> > is still under test. So, should we apply the same patch
> > to master as well as cygwin-3_3-branch?
> 
> Yes, I think so.  Then we can rebase topic/pipe onto master.

How can I rebase topic/pipe to master?
Are the following steps right manner?

git switch topic/pipe
git rebase master
[edit fhandler_pipe.cc to resolve conflict.]
git add winsup/cygwin/fhandler_pipe.cc
git rebase --continue


-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: cygwin 3.3.x: another problem that may be related to pipes
  2021-11-16  3:28                     ` Takashi Yano
@ 2021-11-16  9:28                       ` Corinna Vinschen
  0 siblings, 0 replies; 20+ messages in thread
From: Corinna Vinschen @ 2021-11-16  9:28 UTC (permalink / raw)
  To: cygwin-developers

On Nov 16 12:28, Takashi Yano wrote:
> On Mon, 15 Nov 2021 18:49:34 -0500
> Ken Brown wrote:
> > On 11/15/2021 6:35 PM, Takashi Yano wrote:
> > > On Mon, 15 Nov 2021 13:47:44 -0500
> > > Ken Brown wrote:
> > >> On 11/15/2021 11:52 AM, Takashi Yano wrote:
> > >>> So, currently we have three options.
> > >>>
> > >>> 1) Call CancelIo() immediately after STATUS_PENDING like my patch.
> > >>> 2) Wait for pipe handle after STATUS_PENDING for nonblocking mode
> > >>>      like Ken's patch.
> > >>> 3) Create evt event even for nonblocking mode as Corinna mentioned.
> > >>>
> > >>> Which is the best solution, do you think?
> > >>
> > >> I'm completely unbiased, of course, but I like option 2.
> > > 
> > > OK. Shall I make a patch? Or would you like to do that?
> > 
> > If you don't mind doing it, I'd appreciate it.  I have a lot of Real Life things 
> > going on at the moment and might not get to it for a couple days.
> 
> OK. I submitted the patch.
> 
> > BTW, I think we have to do the same thing for fhandler_fifo::raw_read, but you 
> > can leave that for me.
> > 
> > > What should we do for master branch? I think topic/pipe
> > > is still under test. So, should we apply the same patch
> > > to master as well as cygwin-3_3-branch?
> > 
> > Yes, I think so.  Then we can rebase topic/pipe onto master.
> 
> How can I rebase topic/pipe to master?
> Are the following steps right manner?
> 
> git switch topic/pipe
> git rebase master

I always use --interactive here, but...

> [edit fhandler_pipe.cc to resolve conflict.]
> git add winsup/cygwin/fhandler_pipe.cc
> git rebase --continue

...otherwise looks right.


Corinna

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: cygwin 3.3.x: another problem that may be related to pipes
  2021-11-15 23:49                   ` Ken Brown
  2021-11-16  3:28                     ` Takashi Yano
@ 2021-11-16  9:45                     ` Takashi Yano
  2021-11-16 10:20                       ` Takashi Yano
  1 sibling, 1 reply; 20+ messages in thread
From: Takashi Yano @ 2021-11-16  9:45 UTC (permalink / raw)
  To: cygwin-developers

On Mon, 15 Nov 2021 18:49:34 -0500
Ken Brown wrote:
> On 11/15/2021 6:35 PM, Takashi Yano wrote:
> > On Mon, 15 Nov 2021 13:47:44 -0500
> > Ken Brown wrote:
> >> On 11/15/2021 11:52 AM, Takashi Yano wrote:
> >>> So, currently we have three options.
> >>>
> >>> 1) Call CancelIo() immediately after STATUS_PENDING like my patch.
> >>> 2) Wait for pipe handle after STATUS_PENDING for nonblocking mode
> >>>      like Ken's patch.
> >>> 3) Create evt event even for nonblocking mode as Corinna mentioned.
> >>>
> >>> Which is the best solution, do you think?
> >>
> >> I'm completely unbiased, of course, but I like option 2.
> > 
> > OK. Shall I make a patch? Or would you like to do that?
> 
> If you don't mind doing it, I'd appreciate it.  I have a lot of Real Life things 
> going on at the moment and might not get to it for a couple days.

Hi Ken,

I noticed the following while fixing this issue.

We now noticed that NtWriteFile() may return STATUS_PENDING
even in nonblocking mode.

If NtWriteFile() returns STATUS_PENDING in nonblocking mode,
does not the following 'if' block in raw_write() refer to 
uninitialized io.Information?

      while (len1 > 0)
        {
          status = NtWriteFile (get_handle (), evt, NULL, NULL, &io,
                                (PVOID) ptr, len1, NULL, NULL);
          if (evt || !NT_SUCCESS (status) || io.Information > 0
              || len <= PIPE_BUF)
            break;
          len1 >>= 1;
        }

'evt' is false if we are in nonblocking mode.
'!NT_SUCCESS(status)' is false if status == STATUS_PENDING.
Then io.Information would be referred I think.

Isn't this another bug in raw_write()?

What should we do in this case? Should we do like:

          if (evt || !NT_SUCCESS (status) || status == STATUS_PENDING
              || io.Information > 0 || len <= PIPE_BUF)

?

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: cygwin 3.3.x: another problem that may be related to pipes
  2021-11-16  9:45                     ` Takashi Yano
@ 2021-11-16 10:20                       ` Takashi Yano
  2021-11-16 14:35                         ` Takashi Yano
  0 siblings, 1 reply; 20+ messages in thread
From: Takashi Yano @ 2021-11-16 10:20 UTC (permalink / raw)
  To: cygwin-developers

On Tue, 16 Nov 2021 18:45:15 +0900
Takashi Yano wrote:
> Hi Ken,
> 
> I noticed the following while fixing this issue.
> 
> We now noticed that NtWriteFile() may return STATUS_PENDING
> even in nonblocking mode.
> 
> If NtWriteFile() returns STATUS_PENDING in nonblocking mode,
> does not the following 'if' block in raw_write() refer to 
> uninitialized io.Information?
> 
>       while (len1 > 0)
>         {
>           status = NtWriteFile (get_handle (), evt, NULL, NULL, &io,
>                                 (PVOID) ptr, len1, NULL, NULL);
>           if (evt || !NT_SUCCESS (status) || io.Information > 0
>               || len <= PIPE_BUF)
>             break;
>           len1 >>= 1;
>         }
> 
> 'evt' is false if we are in nonblocking mode.
> '!NT_SUCCESS(status)' is false if status == STATUS_PENDING.
> Then io.Information would be referred I think.
> 
> Isn't this another bug in raw_write()?
> 
> What should we do in this case? Should we do like:
> 
>           if (evt || !NT_SUCCESS (status) || status == STATUS_PENDING
>               || io.Information > 0 || len <= PIPE_BUF)
> 
> ?

Answer to myself.

I think moving cygwait(evt, ...) before this 'if' block is the
right thing.

Right? If so, I will submit v3 patch.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: cygwin 3.3.x: another problem that may be related to pipes
  2021-11-16 10:20                       ` Takashi Yano
@ 2021-11-16 14:35                         ` Takashi Yano
  0 siblings, 0 replies; 20+ messages in thread
From: Takashi Yano @ 2021-11-16 14:35 UTC (permalink / raw)
  To: cygwin-developers

On Tue, 16 Nov 2021 19:20:15 +0900
Takashi Yano wrote:
> On Tue, 16 Nov 2021 18:45:15 +0900
> Takashi Yano wrote:
> > Hi Ken,
> > 
> > I noticed the following while fixing this issue.
> > 
> > We now noticed that NtWriteFile() may return STATUS_PENDING
> > even in nonblocking mode.
> > 
> > If NtWriteFile() returns STATUS_PENDING in nonblocking mode,
> > does not the following 'if' block in raw_write() refer to 
> > uninitialized io.Information?
> > 
> >       while (len1 > 0)
> >         {
> >           status = NtWriteFile (get_handle (), evt, NULL, NULL, &io,
> >                                 (PVOID) ptr, len1, NULL, NULL);
> >           if (evt || !NT_SUCCESS (status) || io.Information > 0
> >               || len <= PIPE_BUF)
> >             break;
> >           len1 >>= 1;
> >         }
> > 
> > 'evt' is false if we are in nonblocking mode.
> > '!NT_SUCCESS(status)' is false if status == STATUS_PENDING.
> > Then io.Information would be referred I think.
> > 
> > Isn't this another bug in raw_write()?
> > 
> > What should we do in this case? Should we do like:
> > 
> >           if (evt || !NT_SUCCESS (status) || status == STATUS_PENDING
> >               || io.Information > 0 || len <= PIPE_BUF)
> > 
> > ?
> 
> Answer to myself.
> 
> I think moving cygwait(evt, ...) before this 'if' block is the
> right thing.
> 
> Right? If so, I will submit v3 patch.

I have just submitted v3 patch. Please have a look.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2021-11-16 14:43 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <115203324.649908.1636923059546.ref@mail.yahoo.com>
     [not found] ` <115203324.649908.1636923059546@mail.yahoo.com>
     [not found]   ` <20211115171811.844dce9cce2b4d13262d64f2@nifty.ne.jp>
2021-11-15 13:01     ` cygwin 3.3.x: another problem that may be related to pipes Corinna Vinschen
2021-11-15 13:57       ` Ken Brown
2021-11-15 14:36         ` Takashi Yano
2021-11-15 15:11           ` Takashi Yano
2021-11-15 15:27             ` Ken Brown
2021-11-15 15:36               ` Ken Brown
2021-11-15 16:45                 ` Takashi Yano
2021-11-15 16:25           ` Corinna Vinschen
2021-11-15 14:50       ` Takashi Yano
2021-11-15 16:08         ` Corinna Vinschen
2021-11-15 16:37           ` Ken Brown
2021-11-15 16:52             ` Takashi Yano
2021-11-15 18:47               ` Ken Brown
2021-11-15 23:35                 ` Takashi Yano
2021-11-15 23:49                   ` Ken Brown
2021-11-16  3:28                     ` Takashi Yano
2021-11-16  9:28                       ` Corinna Vinschen
2021-11-16  9:45                     ` Takashi Yano
2021-11-16 10:20                       ` Takashi Yano
2021-11-16 14:35                         ` Takashi Yano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).