public inbox for cygwin-developers@cygwin.com
 help / color / mirror / Atom feed
* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
       [not found]           ` <2cfa5de7-3b95-9062-4572-f36d304bc916@cornell.edu>
@ 2021-11-06  6:10             ` Takashi Yano
  2021-11-06 11:42               ` Corinna Vinschen
  0 siblings, 1 reply; 33+ messages in thread
From: Takashi Yano @ 2021-11-06  6:10 UTC (permalink / raw)
  To: cygwin-developers

# This topic is moved from cygwin@cygwin.com mailing list.

Hi Ken,

On Fri, 5 Nov 2021 16:08:31 -0400
Ken Brown wrote:
> On 11/5/2021 3:41 PM, Takashi Yano via Cygwin wrote:
> > Thanks much for the detailed steps. I could reproduce the problem.
> > 
> > It seems that the cause is the overhaul for the pipe implementation.
> > I also found the workaround for this issue. Please try:
> > export CYGWIN=pipe_byte
> > 
> > Corinna, Ken,
> > What about setting pipe mode to byte by default?
> 
> I have a vague recollection that this caused some other problem, but I'll have 
> to review the email discussions from a few months ago.  I'm traveling at the 
> moment and won't get to this for a few days.
> 
> In the meantime, could you explain (probably on cygwin-developers) why message 
> mode causes the reported issue?  Also, does the problem occur only when there 
> are non-Cygwin apps involved?

I digged deeper this problem and might find the cause.

Perhaps, C# program sometimes writes 0 byte to stdout.
As a result, if stdout is a message pipe, raw_read() returns
0 byte and EOF is falsely detected. So, setting pipe type
to byte solves the issue.

I used the following two test programs. The output of
second test program (do_pipe) is like:
res=1
len=0          <<<<< Look here!!
res=1
len=14
res=0
len=0


/* C# program which is supposed to write 0 byte msg. */
/* Filename: produce.cs */
/* Compile this with "csc produce.cs" */
using System;
namespace Produce
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine("Hello World.");
        }
    }
}
/* End of produce.cs */


/* Filename: do_pipe.c */
/* Compile this with "x86_64-w64-mingw32-gcc do_pipe.c -o do_pipe" */
#include <windows.h>
#include <stdio.h>

#if 0
#define PTYPE PIPE_TYPE_BYTE
#else
#define PTYPE PIPE_TYPE_MESSAGE
#endif

int main()
{
	HANDLE hr, hw;
	STARTUPINFO si;
	PROCESS_INFORMATION pi;
	char buf[1024];
	BOOL res;

	SECURITY_ATTRIBUTES sec_none = {sizeof(SECURITY_ATTRIBUTES), NULL, TRUE};
	hr = CreateNamedPipe("\\\\.\\pipe\\pipe-test1",
			PIPE_ACCESS_INBOUND, PTYPE,
			1, 4096, 4096, NMPWAIT_USE_DEFAULT_WAIT, &sec_none);
	hw = CreateFile("\\\\.\\pipe\\pipe-test1",
			GENERIC_WRITE | FILE_READ_ATTRIBUTES, 0, &sec_none,
			OPEN_EXISTING, 0, 0);

	ZeroMemory(&si, sizeof(si));
	si.cb = sizeof(si);
	si.dwFlags = STARTF_USESTDHANDLES;
	si.hStdInput = GetStdHandle(STD_INPUT_HANDLE);
	si.hStdOutput = hw;
	si.hStdError = GetStdHandle(STD_ERROR_HANDLE);
	CreateProcess(NULL, "produce", NULL, NULL, TRUE, 0, NULL, NULL, &si, &pi);

	CloseHandle(hw);

	do {
		DWORD len;
		res = ReadFile(hr, buf, sizeof(buf), &len, NULL); 
		printf("res=%d\n", res);
		printf("len=%d\n", len);
	} while (res);

	CloseHandle(hr);

	WaitForSingleObject(pi.hProcess, INFINITE);
	return 0;
}
/* End of do_pipe.c */


Therefore, the following patch instead of setting pipe type to byte
also resolves the issue.

diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
index 43771e8f7..bc06d157c 100644
--- a/winsup/cygwin/fhandler_pipe.cc
+++ b/winsup/cygwin/fhandler_pipe.cc
@@ -393,7 +393,8 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
 	    }
 	}
 
-      if (nbytes_now == 0 || status == STATUS_BUFFER_OVERFLOW)
+      if ((nbytes_now == 0 && !NT_SUCCESS (status))
+	  || status == STATUS_BUFFER_OVERFLOW)
 	break;
     }
   ReleaseMutex (read_mtx);

I am not sure which is the better solution.


P.S.
Unfortunately, these solutions do not resolve the issue
which is another issue with C# program:
https://cygwin.com/pipermail/cygwin/2021-March/247987.html
This still needs FILE_SYNCHRONOUS_IO_NONALERT flag.

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

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-06  6:10             ` 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot Takashi Yano
@ 2021-11-06 11:42               ` Corinna Vinschen
  2021-11-06 12:02                 ` Corinna Vinschen
                                   ` (2 more replies)
  0 siblings, 3 replies; 33+ messages in thread
From: Corinna Vinschen @ 2021-11-06 11:42 UTC (permalink / raw)
  To: cygwin-developers

On Nov  6 15:10, Takashi Yano wrote:
> # This topic is moved from cygwin@cygwin.com mailing list.
> 
> Hi Ken,
> 
> On Fri, 5 Nov 2021 16:08:31 -0400
> Ken Brown wrote:
> > On 11/5/2021 3:41 PM, Takashi Yano via Cygwin wrote:
> > > Thanks much for the detailed steps. I could reproduce the problem.
> > > 
> > > It seems that the cause is the overhaul for the pipe implementation.
> > > I also found the workaround for this issue. Please try:
> > > export CYGWIN=pipe_byte
> > > 
> > > Corinna, Ken,
> > > What about setting pipe mode to byte by default?
> > 
> > I have a vague recollection that this caused some other problem, but I'll have 
> > to review the email discussions from a few months ago.  I'm traveling at the 
> > moment and won't get to this for a few days.
> > 
> > In the meantime, could you explain (probably on cygwin-developers) why message 
> > mode causes the reported issue?  Also, does the problem occur only when there 
> > are non-Cygwin apps involved?
> 
> I digged deeper this problem and might find the cause.
> 
> Perhaps, C# program sometimes writes 0 byte to stdout.
> As a result, if stdout is a message pipe, raw_read() returns
> 0 byte and EOF is falsely detected. So, setting pipe type
> to byte solves the issue.

Given that EOF is reported as status STATUS_END_OF_FILE, wouldn't it
make sense to just check for 0 bytes in raw_read and continue the loop
as if literally nothing has happened?

> diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
> index 43771e8f7..bc06d157c 100644
> --- a/winsup/cygwin/fhandler_pipe.cc
> +++ b/winsup/cygwin/fhandler_pipe.cc
> @@ -393,7 +393,8 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
>  	    }
>  	}
>  
> -      if (nbytes_now == 0 || status == STATUS_BUFFER_OVERFLOW)
> +      if ((nbytes_now == 0 && !NT_SUCCESS (status))
> +	  || status == STATUS_BUFFER_OVERFLOW)
>  	break;
>      }
>    ReleaseMutex (read_mtx);

Oh, heh.

> I am not sure which is the better solution.

Me neither.  I remember that using a message type pipe fixed some
problem of old, but just because message pipes were a solution in the
past...  A quick search uncovered that the message type pipes have been
introduced in April 2012.  We might have to search the mailing list
archives to fiund the reason.

The question is if that problem is still present in the overhauled code
at all.

> P.S.
> Unfortunately, these solutions do not resolve the issue
> which is another issue with C# program:
> https://cygwin.com/pipermail/cygwin/2021-March/247987.html
> This still needs FILE_SYNCHRONOUS_IO_NONALERT flag.

If we want to add FILE_SYNCHRONOUS_IO_NONALERT, this would have to be
solved by running NtReadFile/NtWriteFile synchronously in a thread,
started on every invocation of raw_read/raw_write.  raw_read/raw_write
would then call cygwait on the thread object.  To break on signal or
thread cancallation events, it would have to call CancelSynchronousIo.
That's certainly doable.


Corinna

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-06 11:42               ` Corinna Vinschen
@ 2021-11-06 12:02                 ` Corinna Vinschen
  2021-11-06 14:13                   ` Takashi Yano
  2021-11-06 16:38                 ` Ken Brown
  2021-11-10  8:30                 ` Takashi Yano
  2 siblings, 1 reply; 33+ messages in thread
From: Corinna Vinschen @ 2021-11-06 12:02 UTC (permalink / raw)
  To: cygwin-developers

On Nov  6 12:42, Corinna Vinschen wrote:
> On Nov  6 15:10, Takashi Yano wrote:
> > P.S.
> > Unfortunately, these solutions do not resolve the issue
> > which is another issue with C# program:
> > https://cygwin.com/pipermail/cygwin/2021-March/247987.html
> > This still needs FILE_SYNCHRONOUS_IO_NONALERT flag.
> 
> If we want to add FILE_SYNCHRONOUS_IO_NONALERT, this would have to be
> solved by running NtReadFile/NtWriteFile synchronously in a thread,
> started on every invocation of raw_read/raw_write.  raw_read/raw_write
> would then call cygwait on the thread object.  To break on signal or
> thread cancallation events, it would have to call CancelSynchronousIo.
> That's certainly doable.

That would be something for 3.4, though.  For 3.3.2, we should just
fix the other problem.  Ignoring 0 byte packets is probably the most
easy way out.

With this fixed, I think I should release 3.3.2 soon.  We can have as
much 3.3 bugfix releases as we want, anyway.


Thanks,
Corinna

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-06 12:02                 ` Corinna Vinschen
@ 2021-11-06 14:13                   ` Takashi Yano
  2021-11-06 17:20                     ` Corinna Vinschen
  0 siblings, 1 reply; 33+ messages in thread
From: Takashi Yano @ 2021-11-06 14:13 UTC (permalink / raw)
  To: cygwin-developers

On Sat, 6 Nov 2021 13:02:50 +0100
Corinna Vinschen wrote:
> On Nov  6 12:42, Corinna Vinschen wrote:
> > On Nov  6 15:10, Takashi Yano wrote:
> > > P.S.
> > > Unfortunately, these solutions do not resolve the issue
> > > which is another issue with C# program:
> > > https://cygwin.com/pipermail/cygwin/2021-March/247987.html
> > > This still needs FILE_SYNCHRONOUS_IO_NONALERT flag.
> > 
> > If we want to add FILE_SYNCHRONOUS_IO_NONALERT, this would have to be
> > solved by running NtReadFile/NtWriteFile synchronously in a thread,
> > started on every invocation of raw_read/raw_write.  raw_read/raw_write
> > would then call cygwait on the thread object.  To break on signal or
> > thread cancallation events, it would have to call CancelSynchronousIo.
> > That's certainly doable.
> 
> That would be something for 3.4, though.  For 3.3.2, we should just
> fix the other problem.  Ignoring 0 byte packets is probably the most
> easy way out.
> 
> With this fixed, I think I should release 3.3.2 soon.  We can have as
> much 3.3 bugfix releases as we want, anyway.

We are not sure at this time if the byte pipe causes some problems.

So, I think it is better to adopt the idea of ignoring 0 byte
messages for the time being, and take some time to consider
adopting the byte pipe and enabling FLAG_SYNCHRONOUS_IO_NONALERT.

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

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-06 11:42               ` Corinna Vinschen
  2021-11-06 12:02                 ` Corinna Vinschen
@ 2021-11-06 16:38                 ` Ken Brown
  2021-11-06 17:20                   ` Corinna Vinschen
  2021-11-07  3:46                   ` Takashi Yano
  2021-11-10  8:30                 ` Takashi Yano
  2 siblings, 2 replies; 33+ messages in thread
From: Ken Brown @ 2021-11-06 16:38 UTC (permalink / raw)
  To: cygwin-developers

[Repeating my reply, which I sent to the wrong list.]

On 11/6/2021 7:42 AM, Corinna Vinschen wrote:
> On Nov  6 15:10, Takashi Yano wrote:
>> Ken Brown wrote:
>>> On 11/5/2021 3:41 PM, Takashi Yano via Cygwin wrote:

>>>> What about setting pipe mode to byte by default?
>>>
>>> I have a vague recollection that this caused some other problem, but I'll have
>>> to review the email discussions from a few months ago.  I'm traveling at the
>>> moment and won't get to this for a few days.

I found it:

   https://cygwin.com/pipermail/cygwin-developers/2021-August/012219.html

Ken

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-06 14:13                   ` Takashi Yano
@ 2021-11-06 17:20                     ` Corinna Vinschen
  2021-11-07  3:01                       ` Takashi Yano
  0 siblings, 1 reply; 33+ messages in thread
From: Corinna Vinschen @ 2021-11-06 17:20 UTC (permalink / raw)
  To: cygwin-developers

On Nov  6 23:13, Takashi Yano wrote:
> On Sat, 6 Nov 2021 13:02:50 +0100
> Corinna Vinschen wrote:
> > On Nov  6 12:42, Corinna Vinschen wrote:
> > > On Nov  6 15:10, Takashi Yano wrote:
> > > > P.S.
> > > > Unfortunately, these solutions do not resolve the issue
> > > > which is another issue with C# program:
> > > > https://cygwin.com/pipermail/cygwin/2021-March/247987.html
> > > > This still needs FILE_SYNCHRONOUS_IO_NONALERT flag.
> > > 
> > > If we want to add FILE_SYNCHRONOUS_IO_NONALERT, this would have to be
> > > solved by running NtReadFile/NtWriteFile synchronously in a thread,
> > > started on every invocation of raw_read/raw_write.  raw_read/raw_write
> > > would then call cygwait on the thread object.  To break on signal or
> > > thread cancallation events, it would have to call CancelSynchronousIo.
> > > That's certainly doable.
> > 
> > That would be something for 3.4, though.  For 3.3.2, we should just
> > fix the other problem.  Ignoring 0 byte packets is probably the most
> > easy way out.
> > 
> > With this fixed, I think I should release 3.3.2 soon.  We can have as
> > much 3.3 bugfix releases as we want, anyway.
> 
> We are not sure at this time if the byte pipe causes some problems.
> 
> So, I think it is better to adopt the idea of ignoring 0 byte
> messages for the time being,

Sure enough.  Are you going to send the patch to cygwin-patches?

> and take some time to consider
> adopting the byte pipe and enabling FLAG_SYNCHRONOUS_IO_NONALERT.

We probably don't really need byte pipe, using a synchroneous pipe
and threading should already do the trick, no?


Corinna

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-06 16:38                 ` Ken Brown
@ 2021-11-06 17:20                   ` Corinna Vinschen
  2021-11-07  3:46                   ` Takashi Yano
  1 sibling, 0 replies; 33+ messages in thread
From: Corinna Vinschen @ 2021-11-06 17:20 UTC (permalink / raw)
  To: cygwin-developers

On Nov  6 12:38, Ken Brown wrote:
> [Repeating my reply, which I sent to the wrong list.]
> 
> On 11/6/2021 7:42 AM, Corinna Vinschen wrote:
> > On Nov  6 15:10, Takashi Yano wrote:
> > > Ken Brown wrote:
> > > > On 11/5/2021 3:41 PM, Takashi Yano via Cygwin wrote:
> 
> > > > > What about setting pipe mode to byte by default?
> > > > 
> > > > I have a vague recollection that this caused some other problem, but I'll have
> > > > to review the email discussions from a few months ago.  I'm traveling at the
> > > > moment and won't get to this for a few days.
> 
> I found it:
> 
>   https://cygwin.com/pipermail/cygwin-developers/2021-August/012219.html

Ah, right.  Thx!


Corinna

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-06 17:20                     ` Corinna Vinschen
@ 2021-11-07  3:01                       ` Takashi Yano
  0 siblings, 0 replies; 33+ messages in thread
From: Takashi Yano @ 2021-11-07  3:01 UTC (permalink / raw)
  To: cygwin-developers

On Sat, 6 Nov 2021 18:20:03 +0100
Corinna Vinschen wrote:
> On Nov  6 23:13, Takashi Yano wrote:
> > On Sat, 6 Nov 2021 13:02:50 +0100
> > Corinna Vinschen wrote:
> > > On Nov  6 12:42, Corinna Vinschen wrote:
> > > > On Nov  6 15:10, Takashi Yano wrote:
> > > > > P.S.
> > > > > Unfortunately, these solutions do not resolve the issue
> > > > > which is another issue with C# program:
> > > > > https://cygwin.com/pipermail/cygwin/2021-March/247987.html
> > > > > This still needs FILE_SYNCHRONOUS_IO_NONALERT flag.
> > > > 
> > > > If we want to add FILE_SYNCHRONOUS_IO_NONALERT, this would have to be
> > > > solved by running NtReadFile/NtWriteFile synchronously in a thread,
> > > > started on every invocation of raw_read/raw_write.  raw_read/raw_write
> > > > would then call cygwait on the thread object.  To break on signal or
> > > > thread cancallation events, it would have to call CancelSynchronousIo.
> > > > That's certainly doable.
> > > 
> > > That would be something for 3.4, though.  For 3.3.2, we should just
> > > fix the other problem.  Ignoring 0 byte packets is probably the most
> > > easy way out.
> > > 
> > > With this fixed, I think I should release 3.3.2 soon.  We can have as
> > > much 3.3 bugfix releases as we want, anyway.
> > 
> > We are not sure at this time if the byte pipe causes some problems.
> > 
> > So, I think it is better to adopt the idea of ignoring 0 byte
> > messages for the time being,
> 
> Sure enough.  Are you going to send the patch to cygwin-patches?

All right. I will submit a patch shortly.

> > and take some time to consider
> > adopting the byte pipe and enabling FLAG_SYNCHRONOUS_IO_NONALERT.
> 
> We probably don't really need byte pipe, using a synchroneous pipe
> and threading should already do the trick, no?

As I wrote in
https://cygwin.com/pipermail/cygwin/2021-March/247987.html
C# program seems to need not only FLAG_SYNCHRONOUS_IO_NONALERT
but also PIPE_TYPE_BYTE.

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

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-06 16:38                 ` Ken Brown
  2021-11-06 17:20                   ` Corinna Vinschen
@ 2021-11-07  3:46                   ` Takashi Yano
  2021-11-07 22:20                     ` Ken Brown
  1 sibling, 1 reply; 33+ messages in thread
From: Takashi Yano @ 2021-11-07  3:46 UTC (permalink / raw)
  To: cygwin-developers

On Sat, 6 Nov 2021 12:38:43 -0400
Ken Brown wrote:
> On 11/6/2021 7:42 AM, Corinna Vinschen wrote:
> > On Nov  6 15:10, Takashi Yano wrote:
> >> Ken Brown wrote:
> >>> On 11/5/2021 3:41 PM, Takashi Yano via Cygwin wrote:
> >>>> What about setting pipe mode to byte by default?
> >>>
> >>> I have a vague recollection that this caused some other problem, but I'll have
> >>> to review the email discussions from a few months ago.  I'm traveling at the
> >>> moment and won't get to this for a few days.
> 
> I found it:
> 
>    https://cygwin.com/pipermail/cygwin-developers/2021-August/012219.html

Thanks!

I tested the current pipe behaviour using the following test code.
With both message and byte pipe, the result are the same:
w:65536
r:2048
w:-1
which complies with POSIX requirement.
I have tested under Windows Vista, 7 and 10 and got same results.


#include <unistd.h>
#include <stdio.h>
#include <limits.h>
#include <fcntl.h>

#define PIPE_SIZE 65536

int main()
{
	int fd[2];
	char buf[PIPE_SIZE];
	int flags;

	pipe(fd);

	/* Set non-blocking */
	flags = fcntl(fd[1], F_GETFL);
	flags |= O_NONBLOCK;
	fcntl(fd[1], F_SETFL, flags);

	/* Fill pipe */
	printf("w:%d\n", write(fd[1], buf, PIPE_SIZE));
	/* Free PIPE_BUF/2 bytes */
	printf("r:%d\n", read(fd[0], buf, PIPE_BUF/2));
	/* Write PIPE_BUF bytes */
	printf("w:%d\n", write(fd[1], buf, PIPE_BUF));

	return 0;
}

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

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-07  3:46                   ` Takashi Yano
@ 2021-11-07 22:20                     ` Ken Brown
  2021-11-08  8:23                       ` Takashi Yano
  0 siblings, 1 reply; 33+ messages in thread
From: Ken Brown @ 2021-11-07 22:20 UTC (permalink / raw)
  To: cygwin-developers

On 11/6/2021 11:46 PM, Takashi Yano wrote:
> On Sat, 6 Nov 2021 12:38:43 -0400
> Ken Brown wrote:
>> On 11/6/2021 7:42 AM, Corinna Vinschen wrote:
>>> On Nov  6 15:10, Takashi Yano wrote:
>>>> Ken Brown wrote:
>>>>> On 11/5/2021 3:41 PM, Takashi Yano via Cygwin wrote:
>>>>>> What about setting pipe mode to byte by default?
>>>>>
>>>>> I have a vague recollection that this caused some other problem, but I'll have
>>>>> to review the email discussions from a few months ago.  I'm traveling at the
>>>>> moment and won't get to this for a few days.
>>
>> I found it:
>>
>>     https://cygwin.com/pipermail/cygwin-developers/2021-August/012219.html
> 
> Thanks!
> 
> I tested the current pipe behaviour using the following test code.
> With both message and byte pipe, the result are the same:
> w:65536
> r:2048
> w:-1
> which complies with POSIX requirement.
> I have tested under Windows Vista, 7 and 10 and got same results.

Thanks.  So maybe byte mode is OK after all.

In the meantime I found the commit that changed the default from byte mode to 
message mode:

commit 31d2bedc585420092eb53895c5f5646651f13215
Author: Christopher Faylor <me@cgf.cx>
Date:   Sun Oct 23 19:01:47 2011 +0000

     * fhandler_tty.cc (fhandler_pty_slave::read): Use consistent way for testing
     ReadFile return.
     * pipe.cc (fhandler_pipe::create_selectable): Open the write side of the pipe
     in message-mode to force writing as "chunks".  Explain why.

[...]
--- a/winsup/cygwin/pipe.cc
+++ b/winsup/cygwin/pipe.cc
@@ -238,9 +238,15 @@ fhandler_pipe::create_selectable (LPSECURITY_ATTRIBUTES 
sa_ptr, HANDLE& r,
[...]
+        Note that the write side of the pipe is opened as PIPE_TYPE_MESSAGE.
+        This *seems* to more closely mimic Linux pipe behavior and is
+        definitely required for pty handling since fhandler_pty_master
+        writes to the pipe in chunks, terminated by newline when CANON mode
+        is specified.  */
        r = CreateNamedPipe (pipename, PIPE_ACCESS_INBOUND | overlapped,
-                          PIPE_TYPE_BYTE | PIPE_READMODE_BYTE, 1, psize,
+                          PIPE_TYPE_MESSAGE | PIPE_READMODE_BYTE, 1, psize,
                            psize, NMPWAIT_USE_DEFAULT_WAIT, sa_ptr);

        if (r != INVALID_HANDLE_VALUE)

I also found discussion on the mailing list on the same day as this commit in 
which cgf referred to fixing a bug (that *I* reported, coincidentally):

   https://cygwin.com/pipermail/cygwin/2011-October/197953.html

I'll try to test more thoroughly tomorrow, but a quick test seems to show that 
the STC I submitted in connection with that bug report now succeeds even with 
CYGWIN=pipe_byte.

Ken

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-07 22:20                     ` Ken Brown
@ 2021-11-08  8:23                       ` Takashi Yano
  2021-11-08  9:46                         ` Corinna Vinschen
  0 siblings, 1 reply; 33+ messages in thread
From: Takashi Yano @ 2021-11-08  8:23 UTC (permalink / raw)
  To: cygwin-developers

On Sun, 7 Nov 2021 17:20:46 -0500
Ken Brown wrote:
> In the meantime I found the commit that changed the default from byte mode to 
> message mode:
> 
> commit 31d2bedc585420092eb53895c5f5646651f13215
> Author: Christopher Faylor <me@cgf.cx>
> Date:   Sun Oct 23 19:01:47 2011 +0000
> 
>      * fhandler_tty.cc (fhandler_pty_slave::read): Use consistent way for testing
>      ReadFile return.
>      * pipe.cc (fhandler_pipe::create_selectable): Open the write side of the pipe
>      in message-mode to force writing as "chunks".  Explain why.
> 
> [...]
> --- a/winsup/cygwin/pipe.cc
> +++ b/winsup/cygwin/pipe.cc
> @@ -238,9 +238,15 @@ fhandler_pipe::create_selectable (LPSECURITY_ATTRIBUTES 
> sa_ptr, HANDLE& r,
> [...]
> +        Note that the write side of the pipe is opened as PIPE_TYPE_MESSAGE.
> +        This *seems* to more closely mimic Linux pipe behavior and is
> +        definitely required for pty handling since fhandler_pty_master
> +        writes to the pipe in chunks, terminated by newline when CANON mode
> +        is specified.  */
>         r = CreateNamedPipe (pipename, PIPE_ACCESS_INBOUND | overlapped,
> -                          PIPE_TYPE_BYTE | PIPE_READMODE_BYTE, 1, psize,
> +                          PIPE_TYPE_MESSAGE | PIPE_READMODE_BYTE, 1, psize,
>                             psize, NMPWAIT_USE_DEFAULT_WAIT, sa_ptr);
> 
>         if (r != INVALID_HANDLE_VALUE)
> 
> I also found discussion on the mailing list on the same day as this commit in 
> which cgf referred to fixing a bug (that *I* reported, coincidentally):
> 
>    https://cygwin.com/pipermail/cygwin/2011-October/197953.html
> 
> I'll try to test more thoroughly tomorrow, but a quick test seems to show that 
> the STC I submitted in connection with that bug report now succeeds even with 
> CYGWIN=pipe_byte.

The STC attached to that post is checking pty behaviour. Currently,
the named pipe for pty is always configured as message type regardless
of CYGWIN=pipe_byte.

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

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-08  8:23                       ` Takashi Yano
@ 2021-11-08  9:46                         ` Corinna Vinschen
  0 siblings, 0 replies; 33+ messages in thread
From: Corinna Vinschen @ 2021-11-08  9:46 UTC (permalink / raw)
  To: cygwin-developers

On Nov  8 17:23, Takashi Yano wrote:
> On Sun, 7 Nov 2021 17:20:46 -0500
> Ken Brown wrote:
> > In the meantime I found the commit that changed the default from byte mode to 
> > message mode:
> > 
> > commit 31d2bedc585420092eb53895c5f5646651f13215
> > Author: Christopher Faylor <me@cgf.cx>
> > Date:   Sun Oct 23 19:01:47 2011 +0000
> > 
> >      * fhandler_tty.cc (fhandler_pty_slave::read): Use consistent way for testing
> >      ReadFile return.
> >      * pipe.cc (fhandler_pipe::create_selectable): Open the write side of the pipe
> >      in message-mode to force writing as "chunks".  Explain why.
> > 
> > [...]
> > I also found discussion on the mailing list on the same day as this commit in 
> > which cgf referred to fixing a bug (that *I* reported, coincidentally):
> > 
> >    https://cygwin.com/pipermail/cygwin/2011-October/197953.html
> > 
> > I'll try to test more thoroughly tomorrow, but a quick test seems to show that 
> > the STC I submitted in connection with that bug report now succeeds even with 
> > CYGWIN=pipe_byte.
> 
> The STC attached to that post is checking pty behaviour. Currently,
> the named pipe for pty is always configured as message type regardless
> of CYGWIN=pipe_byte.

Ok, I guess we proceed with the patch Takashi sent to -patches for 3.3.2.

For the master branch, if you like, go ahead and switch to byte mode and
sync IO.  Or we can do that on the topic/pipe branch and merge into master
at one point, that worked nicely the last time, too.


Corinna

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-06 11:42               ` Corinna Vinschen
  2021-11-06 12:02                 ` Corinna Vinschen
  2021-11-06 16:38                 ` Ken Brown
@ 2021-11-10  8:30                 ` Takashi Yano
  2021-11-10 10:34                   ` Corinna Vinschen
  2 siblings, 1 reply; 33+ messages in thread
From: Takashi Yano @ 2021-11-10  8:30 UTC (permalink / raw)
  To: cygwin-developers

[-- Attachment #1: Type: text/plain, Size: 1500 bytes --]

Hi Corinna,

On Sat, 6 Nov 2021 12:42:46 +0100
Corinna Vinschen wrote:
> On Nov  6 15:10, Takashi Yano wrote:
> > Unfortunately, these solutions do not resolve the issue
> > which is another issue with C# program:
> > https://cygwin.com/pipermail/cygwin/2021-March/247987.html
> > This still needs FILE_SYNCHRONOUS_IO_NONALERT flag.
> 
> If we want to add FILE_SYNCHRONOUS_IO_NONALERT, this would have to be
> solved by running NtReadFile/NtWriteFile synchronously in a thread,
> started on every invocation of raw_read/raw_write.  raw_read/raw_write
> would then call cygwait on the thread object.  To break on signal or
> thread cancallation events, it would have to call CancelSynchronousIo.
> That's certainly doable.

I tried to implement your idea, however, I noticed that
NtQueryObject(ObjectNameInformation) call in
get_query_hdl_per_process() is blocked while reading the
pipe if FIPE_SYNCHRONOUS_IO_NONALERT is set and pipe is
in blocking mode.

So I would like to propose alternative implementation with
FILE_SYNCHRONOUS_IO_NONALERT being set. Please have a look
at attached patch. With this patch, pipe itself in read side
is always set to nonblocking mode and simulate the blocking
behaviour in raw_read(). This can eliminate creating thread
for reading as well as calling CancelSynchronousIo().

Note that setting FILE_SYNCHRONOUS_IO_NONALERT only for read
pipe seems to be enough for C# programs.

What do you think of this implementation?

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

[-- Attachment #2: pipe20211110.patch --]
[-- Type: application/octet-stream, Size: 5801 bytes --]

diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
index 13731437e..41121394e 100644
--- a/winsup/cygwin/fhandler_pipe.cc
+++ b/winsup/cygwin/fhandler_pipe.cc
@@ -90,7 +90,10 @@ fhandler_pipe::init (HANDLE f, DWORD a, mode_t mode, int64_t uniq_id)
   set_ino (uniq_id);
   set_unique_id (uniq_id | !!(mode & GENERIC_WRITE));
   if (opened_properly)
-    set_pipe_non_blocking (is_nonblocking ());
+    /* Set read pipe always nonblocking to allow signal handling
+       even with FILE_SYNCHRONOUS_IO_NONALERT. */
+    set_pipe_non_blocking (get_device () == FH_PIPER ?
+			   true : is_nonblocking ());
   return 1;
 }
 
@@ -264,9 +267,9 @@ fhandler_pipe::release_select_sem (const char *from)
   if (get_dev () == FH_PIPER) /* Number of select() and writer */
     n_release = get_obj_handle_count (select_sem)
       - get_obj_handle_count (read_mtx);
-  else /* Number of select() call */
+  else /* Number of select() call and reader */
     n_release = get_obj_handle_count (select_sem)
-      - get_obj_handle_count (hdl_cnt_mtx);
+      - get_obj_handle_count (get_handle ());
   debug_printf("%s(%s) release %d", from,
 	       get_dev () == FH_PIPER ? "PIPER" : "PIPEW", n_release);
   if (n_release)
@@ -279,19 +282,10 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
   size_t nbytes = 0;
   NTSTATUS status = STATUS_SUCCESS;
   IO_STATUS_BLOCK io;
-  HANDLE evt = NULL;
 
   if (!len)
     return;
 
-  /* Create a wait event if we're in blocking mode. */
-  if (!is_nonblocking () && !(evt = CreateEvent (NULL, false, false, NULL)))
-    {
-      __seterrno ();
-      len = (size_t) -1;
-      return;
-    }
-
   DWORD timeout = is_nonblocking () ? 0 : INFINITE;
   DWORD waitret = cygwait (read_mtx, timeout);
   switch (waitret)
@@ -319,48 +313,23 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
     {
       ULONG_PTR nbytes_now = 0;
       ULONG len1 = (ULONG) (len - nbytes);
-      waitret = WAIT_OBJECT_0;
 
-      if (evt)
-	ResetEvent (evt);
       FILE_PIPE_LOCAL_INFORMATION fpli;
       status = NtQueryInformationFile (get_handle (), &io,
 				       &fpli, sizeof (fpli),
 				       FilePipeLocalInformation);
       if (NT_SUCCESS (status))
 	{
-	if (fpli.ReadDataAvailable == 0 && nbytes != 0)
-	  break;
+	  if (fpli.ReadDataAvailable == 0 && nbytes != 0)
+	    break;
 	}
       else if (nbytes != 0)
 	break;
-      status = NtReadFile (get_handle (), evt, NULL, NULL, &io, ptr,
+      status = NtReadFile (get_handle (), NULL, NULL, NULL, &io, ptr,
 			   len1, NULL, NULL);
-      if (evt && status == STATUS_PENDING)
-	{
-	  waitret = cygwait (evt, 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.
-	     Otherwise IO has been finished regulary and io.Status contains
-	     valid success or error information. */
-	  CancelIo (get_handle ());
-	  if (waitret == WAIT_SIGNALED && io.Status != STATUS_CANCELLED)
-	    waitret = WAIT_OBJECT_0;
-
-	  if (waitret == WAIT_CANCELED)
-	    status = STATUS_THREAD_CANCELED;
-	  else if (waitret == WAIT_SIGNALED)
-	    status = STATUS_THREAD_SIGNALED;
-	  else
-	    status = io.Status;
-	}
       if (isclosed ())  /* A signal handler might have closed the fd. */
 	{
-	  if (waitret == WAIT_OBJECT_0)
-	    set_errno (EBADF);
-	  else
-	    __seterrno ();
+	  set_errno (EBADF);
 	  nbytes = (size_t) -1;
 	}
       else if (NT_SUCCESS (status)
@@ -393,7 +362,16 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
 		  nbytes = (size_t) -1;
 		  break;
 		}
-	      fallthrough;
+	      waitret = cygwait (select_sem, 1);
+	      if (waitret == WAIT_CANCELED)
+		pthread::static_cancel_self ();
+	      else if (waitret == WAIT_SIGNALED)
+		{
+		  set_errno (EINTR);
+		  nbytes = (size_t) -1;
+		  break;
+		}
+	      continue;
 	    default:
 	      __seterrno_from_nt_status (status);
 	      nbytes = (size_t) -1;
@@ -406,8 +384,6 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
 	break;
     }
   ReleaseMutex (read_mtx);
-  if (evt)
-    CloseHandle (evt);
   if (status == STATUS_THREAD_SIGNALED && nbytes == 0)
     {
       set_errno (EINTR);
@@ -993,9 +969,12 @@ nt_create (LPSECURITY_ATTRIBUTES sa_ptr, HANDLE &r, HANDLE &w,
 				  npfsh, sa_ptr->lpSecurityDescriptor);
 
       timeout.QuadPart = -500000;
+      /* Set FILE_SYNCHRONOUS_IO_NONALERT flag so that native
+	 C# programs work with cygwin pipe. */
       status = NtCreateNamedPipeFile (&r, access, &attr, &io,
 				      FILE_SHARE_READ | FILE_SHARE_WRITE,
-				      FILE_CREATE, 0, pipe_type,
+				      FILE_CREATE,
+				      FILE_SYNCHRONOUS_IO_NONALERT, pipe_type,
 				      FILE_PIPE_BYTE_STREAM_MODE,
 				      0, 1, psize, psize, &timeout);
 
@@ -1107,7 +1086,9 @@ fhandler_pipe::fcntl (int cmd, intptr_t arg)
   const bool was_nonblocking = is_nonblocking ();
   int res = fhandler_base::fcntl (cmd, arg);
   const bool now_nonblocking = is_nonblocking ();
-  if (now_nonblocking != was_nonblocking)
+  /* Do not set blocking mode for read pipe to allow signal handling
+     even with FILE_SYNCHRONOUS_IO_NONALERT. */
+  if (now_nonblocking != was_nonblocking && get_device () != FH_PIPER)
     set_pipe_non_blocking (now_nonblocking);
   return res;
 }
diff --git a/winsup/cygwin/globals.cc b/winsup/cygwin/globals.cc
index 48fb312de..ac5ad0307 100644
--- a/winsup/cygwin/globals.cc
+++ b/winsup/cygwin/globals.cc
@@ -69,7 +69,7 @@ int NO_COPY dynamically_loaded;
 /* Some CYGWIN environment variable variables. */
 bool allow_glob = true;
 bool ignore_case_with_glob;
-bool pipe_byte;
+bool pipe_byte = true; /* Default to byte mode so that C# programs work. */
 bool reset_com;
 bool wincmdln;
 winsym_t allow_winsymlinks = WSYM_default;

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-10  8:30                 ` Takashi Yano
@ 2021-11-10 10:34                   ` Corinna Vinschen
  2021-11-10 13:30                     ` Takashi Yano
  0 siblings, 1 reply; 33+ messages in thread
From: Corinna Vinschen @ 2021-11-10 10:34 UTC (permalink / raw)
  To: cygwin-developers

On Nov 10 17:30, Takashi Yano wrote:
> Hi Corinna,
> 
> On Sat, 6 Nov 2021 12:42:46 +0100
> Corinna Vinschen wrote:
> > On Nov  6 15:10, Takashi Yano wrote:
> > > Unfortunately, these solutions do not resolve the issue
> > > which is another issue with C# program:
> > > https://cygwin.com/pipermail/cygwin/2021-March/247987.html
> > > This still needs FILE_SYNCHRONOUS_IO_NONALERT flag.
> > 
> > If we want to add FILE_SYNCHRONOUS_IO_NONALERT, this would have to be
> > solved by running NtReadFile/NtWriteFile synchronously in a thread,
> > started on every invocation of raw_read/raw_write.  raw_read/raw_write
> > would then call cygwait on the thread object.  To break on signal or
> > thread cancallation events, it would have to call CancelSynchronousIo.
> > That's certainly doable.
> 
> I tried to implement your idea, however, I noticed that
> NtQueryObject(ObjectNameInformation) call in
> get_query_hdl_per_process() is blocked while reading the
> pipe if FIPE_SYNCHRONOUS_IO_NONALERT is set and pipe is
> in blocking mode.
> 
> So I would like to propose alternative implementation with
> FILE_SYNCHRONOUS_IO_NONALERT being set. Please have a look
> at attached patch. With this patch, pipe itself in read side
> is always set to nonblocking mode and simulate the blocking
> behaviour in raw_read(). This can eliminate creating thread
> for reading as well as calling CancelSynchronousIo().
> 
> Note that setting FILE_SYNCHRONOUS_IO_NONALERT only for read
> pipe seems to be enough for C# programs.
> 
> What do you think of this implementation?

Can you push this to the topic/pipe branch for playing?  I updated
the branch to current master.


Thanks,
Corinna

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-10 10:34                   ` Corinna Vinschen
@ 2021-11-10 13:30                     ` Takashi Yano
  2021-11-10 20:35                       ` Corinna Vinschen
  0 siblings, 1 reply; 33+ messages in thread
From: Takashi Yano @ 2021-11-10 13:30 UTC (permalink / raw)
  To: cygwin-developers

On Wed, 10 Nov 2021 11:34:25 +0100
Corinna Vinschen wrote:
> On Nov 10 17:30, Takashi Yano wrote:
> > On Sat, 6 Nov 2021 12:42:46 +0100
> > Corinna Vinschen wrote:
> > > On Nov  6 15:10, Takashi Yano wrote:
> > > > Unfortunately, these solutions do not resolve the issue
> > > > which is another issue with C# program:
> > > > https://cygwin.com/pipermail/cygwin/2021-March/247987.html
> > > > This still needs FILE_SYNCHRONOUS_IO_NONALERT flag.
> > > 
> > > If we want to add FILE_SYNCHRONOUS_IO_NONALERT, this would have to be
> > > solved by running NtReadFile/NtWriteFile synchronously in a thread,
> > > started on every invocation of raw_read/raw_write.  raw_read/raw_write
> > > would then call cygwait on the thread object.  To break on signal or
> > > thread cancallation events, it would have to call CancelSynchronousIo.
> > > That's certainly doable.
> > 
> > I tried to implement your idea, however, I noticed that
> > NtQueryObject(ObjectNameInformation) call in
> > get_query_hdl_per_process() is blocked while reading the
> > pipe if FIPE_SYNCHRONOUS_IO_NONALERT is set and pipe is
> > in blocking mode.
> > 
> > So I would like to propose alternative implementation with
> > FILE_SYNCHRONOUS_IO_NONALERT being set. Please have a look
> > at attached patch. With this patch, pipe itself in read side
> > is always set to nonblocking mode and simulate the blocking
> > behaviour in raw_read(). This can eliminate creating thread
> > for reading as well as calling CancelSynchronousIo().
> > 
> > Note that setting FILE_SYNCHRONOUS_IO_NONALERT only for read
> > pipe seems to be enough for C# programs.
> > 
> > What do you think of this implementation?
> 
> Can you push this to the topic/pipe branch for playing?  I updated
> the branch to current master.

Thanks. I have just pushed the experimental patch to topic/pipe.
Please try. If something wrong, please point it out.

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

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-10 13:30                     ` Takashi Yano
@ 2021-11-10 20:35                       ` Corinna Vinschen
  2021-11-10 21:32                         ` Ken Brown
  2021-11-11  9:52                         ` Corinna Vinschen
  0 siblings, 2 replies; 33+ messages in thread
From: Corinna Vinschen @ 2021-11-10 20:35 UTC (permalink / raw)
  To: cygwin-developers

On Nov 10 22:30, Takashi Yano wrote:
> On Wed, 10 Nov 2021 11:34:25 +0100
> Corinna Vinschen wrote:
> > On Nov 10 17:30, Takashi Yano wrote:
> > > On Sat, 6 Nov 2021 12:42:46 +0100
> > > Corinna Vinschen wrote:
> > > > On Nov  6 15:10, Takashi Yano wrote:
> > > > > Unfortunately, these solutions do not resolve the issue
> > > > > which is another issue with C# program:
> > > > > https://cygwin.com/pipermail/cygwin/2021-March/247987.html
> > > > > This still needs FILE_SYNCHRONOUS_IO_NONALERT flag.
> > > > 
> > > > If we want to add FILE_SYNCHRONOUS_IO_NONALERT, this would have to be
> > > > solved by running NtReadFile/NtWriteFile synchronously in a thread,
> > > > started on every invocation of raw_read/raw_write.  raw_read/raw_write
> > > > would then call cygwait on the thread object.  To break on signal or
> > > > thread cancallation events, it would have to call CancelSynchronousIo.
> > > > That's certainly doable.
> > > 
> > > I tried to implement your idea, however, I noticed that
> > > NtQueryObject(ObjectNameInformation) call in
> > > get_query_hdl_per_process() is blocked while reading the
> > > pipe if FIPE_SYNCHRONOUS_IO_NONALERT is set and pipe is
> > > in blocking mode.
> > > 
> > > So I would like to propose alternative implementation with
> > > FILE_SYNCHRONOUS_IO_NONALERT being set. Please have a look
> > > at attached patch. With this patch, pipe itself in read side
> > > is always set to nonblocking mode and simulate the blocking
> > > behaviour in raw_read(). This can eliminate creating thread
> > > for reading as well as calling CancelSynchronousIo().
> > > 
> > > Note that setting FILE_SYNCHRONOUS_IO_NONALERT only for read
> > > pipe seems to be enough for C# programs.
> > > 
> > > What do you think of this implementation?
> > 
> > Can you push this to the topic/pipe branch for playing?  I updated
> > the branch to current master.
> 
> Thanks. I have just pushed the experimental patch to topic/pipe.
> Please try. If something wrong, please point it out.

Great, I'll have a look.  Ken, you're looking as well, right?


Corinna

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-10 20:35                       ` Corinna Vinschen
@ 2021-11-10 21:32                         ` Ken Brown
  2021-11-11 16:11                           ` Ken Brown
  2021-11-11  9:52                         ` Corinna Vinschen
  1 sibling, 1 reply; 33+ messages in thread
From: Ken Brown @ 2021-11-10 21:32 UTC (permalink / raw)
  To: cygwin-developers

On 11/10/2021 3:35 PM, Corinna Vinschen wrote:
> On Nov 10 22:30, Takashi Yano wrote:
>> Thanks. I have just pushed the experimental patch to topic/pipe.
>> Please try. If something wrong, please point it out.
> 
> Great, I'll have a look.  Ken, you're looking as well, right?

Yes.  It looks good on a first read-through.  I want to look again and do some 
testing.  One minor thing I noticed is that fhandler_pipe::raw_read could be 
cleaned up a little more: There are still references to STATUS_THREAD_SIGNALED 
and STATUS_THREAD_CANCELED, which can't occur any more.

Ken

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-10 20:35                       ` Corinna Vinschen
  2021-11-10 21:32                         ` Ken Brown
@ 2021-11-11  9:52                         ` Corinna Vinschen
  2021-11-11 11:12                           ` Takashi Yano
  1 sibling, 1 reply; 33+ messages in thread
From: Corinna Vinschen @ 2021-11-11  9:52 UTC (permalink / raw)
  To: cygwin-developers

On Nov 10 21:35, Corinna Vinschen wrote:
> On Nov 10 22:30, Takashi Yano wrote:
> > On Wed, 10 Nov 2021 11:34:25 +0100
> > Corinna Vinschen wrote:
> > > On Nov 10 17:30, Takashi Yano wrote:
> > > > I tried to implement your idea, however, I noticed that
> > > > NtQueryObject(ObjectNameInformation) call in
> > > > get_query_hdl_per_process() is blocked while reading the
> > > > pipe if FIPE_SYNCHRONOUS_IO_NONALERT is set and pipe is
> > > > in blocking mode.
> > > > 
> > > > So I would like to propose alternative implementation with
> > > > FILE_SYNCHRONOUS_IO_NONALERT being set. Please have a look
> > > > at attached patch. With this patch, pipe itself in read side
> > > > is always set to nonblocking mode and simulate the blocking
> > > > behaviour in raw_read(). This can eliminate creating thread
> > > > for reading as well as calling CancelSynchronousIo().
> > > > 
> > > > Note that setting FILE_SYNCHRONOUS_IO_NONALERT only for read
> > > > pipe seems to be enough for C# programs.
> > > > 
> > > > What do you think of this implementation?
> > > 
> > > Can you push this to the topic/pipe branch for playing?  I updated
> > > the branch to current master.
> > 
> > Thanks. I have just pushed the experimental patch to topic/pipe.
> > Please try. If something wrong, please point it out.
> 
> Great, I'll have a look.  Ken, you're looking as well, right?

What I like with this approach is that it actually simplifies the code.

What I don't like much is that we are now running a busy loop, but the
NtQueryObject hang is... disappointing, to say the least.  What about
using another function like NtQueryInformationFile(FileNameInformation)?
I bet this hangs, too, right?  

Btw., while looking I was wondering if get_query_hdl_per_process and
get_query_hdl_per_system could be replaced by a function utilizing
NtQueryInformationFile(FileProcessIdsUsingFileInformation).  This
supposedly returns all process ids of processes having an open handle to
this file.  This could be quite a bit faster than the other two
functions, ideally.  Per MSDN, FileProcessIdsUsingFileInformation
exists since Vista.


Corinna

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-11  9:52                         ` Corinna Vinschen
@ 2021-11-11 11:12                           ` Takashi Yano
  2021-11-11 11:33                             ` Corinna Vinschen
  2021-11-12  8:33                             ` Takashi Yano
  0 siblings, 2 replies; 33+ messages in thread
From: Takashi Yano @ 2021-11-11 11:12 UTC (permalink / raw)
  To: cygwin-developers

On Thu, 11 Nov 2021 10:52:33 +0100
Corinna Vinschen wrote:
> On Nov 10 21:35, Corinna Vinschen wrote:
> > On Nov 10 22:30, Takashi Yano wrote:
> > > On Wed, 10 Nov 2021 11:34:25 +0100
> > > Corinna Vinschen wrote:
> > > > On Nov 10 17:30, Takashi Yano wrote:
> > > > > I tried to implement your idea, however, I noticed that
> > > > > NtQueryObject(ObjectNameInformation) call in
> > > > > get_query_hdl_per_process() is blocked while reading the
> > > > > pipe if FIPE_SYNCHRONOUS_IO_NONALERT is set and pipe is
> > > > > in blocking mode.
> > > > > 
> > > > > So I would like to propose alternative implementation with
> > > > > FILE_SYNCHRONOUS_IO_NONALERT being set. Please have a look
> > > > > at attached patch. With this patch, pipe itself in read side
> > > > > is always set to nonblocking mode and simulate the blocking
> > > > > behaviour in raw_read(). This can eliminate creating thread
> > > > > for reading as well as calling CancelSynchronousIo().
> > > > > 
> > > > > Note that setting FILE_SYNCHRONOUS_IO_NONALERT only for read
> > > > > pipe seems to be enough for C# programs.
> > > > > 
> > > > > What do you think of this implementation?
> > > > 
> > > > Can you push this to the topic/pipe branch for playing?  I updated
> > > > the branch to current master.
> > > 
> > > Thanks. I have just pushed the experimental patch to topic/pipe.
> > > Please try. If something wrong, please point it out.
> > 
> > Great, I'll have a look.  Ken, you're looking as well, right?
> 
> What I like with this approach is that it actually simplifies the code.
> 
> What I don't like much is that we are now running a busy loop, but the
> NtQueryObject hang is... disappointing, to say the least.  What about
> using another function like NtQueryInformationFile(FileNameInformation)?
> I bet this hangs, too, right?  

I have just confirmed that NtQueryInformationFile(FileNameInformation)
also hangs.

> Btw., while looking I was wondering if get_query_hdl_per_process and
> get_query_hdl_per_system could be replaced by a function utilizing
> NtQueryInformationFile(FileProcessIdsUsingFileInformation).  This
> supposedly returns all process ids of processes having an open handle to
> this file.  This could be quite a bit faster than the other two
> functions, ideally.  Per MSDN, FileProcessIdsUsingFileInformation
> exists since Vista.

Thanks for advice. get_query_hdl_per_* is called in the write side,
so only knows pipe handle of the write side. We would like to know
ProcessId which have pipe handle of the read side. Can we use
FileProcessIdsUsingFileInformation for this perpose?

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

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-11 11:12                           ` Takashi Yano
@ 2021-11-11 11:33                             ` Corinna Vinschen
  2021-11-11 12:02                               ` Takashi Yano
  2021-11-12  8:33                             ` Takashi Yano
  1 sibling, 1 reply; 33+ messages in thread
From: Corinna Vinschen @ 2021-11-11 11:33 UTC (permalink / raw)
  To: cygwin-developers

On Nov 11 20:12, Takashi Yano wrote:
> On Thu, 11 Nov 2021 10:52:33 +0100
> Corinna Vinschen wrote:
> > On Nov 10 21:35, Corinna Vinschen wrote:
> > > On Nov 10 22:30, Takashi Yano wrote:
> > > > On Wed, 10 Nov 2021 11:34:25 +0100
> > > > Corinna Vinschen wrote:
> > > > > On Nov 10 17:30, Takashi Yano wrote:
> > > > > > So I would like to propose alternative implementation with
> > > > > > FILE_SYNCHRONOUS_IO_NONALERT being set. Please have a look
> > > > > > at attached patch. With this patch, pipe itself in read side
> > > > > > is always set to nonblocking mode and simulate the blocking
> > > > > > behaviour in raw_read(). This can eliminate creating thread
> > > > > > for reading as well as calling CancelSynchronousIo().
> > > > > > 
> > > > > > Note that setting FILE_SYNCHRONOUS_IO_NONALERT only for read
> > > > > > pipe seems to be enough for C# programs.
> > > > > > 
> > > > > > What do you think of this implementation?
> > > > > [...]
> > What I like with this approach is that it actually simplifies the code.
> > 
> > What I don't like much is that we are now running a busy loop, but the
> > NtQueryObject hang is... disappointing, to say the least.  What about
> > using another function like NtQueryInformationFile(FileNameInformation)?
> > I bet this hangs, too, right?  
> 
> I have just confirmed that NtQueryInformationFile(FileNameInformation)
> also hangs.

Yeah, drat... but expected.

> > [FileProcessIdsUsingFileInformation]
> 
> Thanks for advice. get_query_hdl_per_* is called in the write side,
> so only knows pipe handle of the write side. We would like to know
> ProcessId which have pipe handle of the read side. Can we use
> FileProcessIdsUsingFileInformation for this perpose?

I don't know.  I just stumbled over it yesterday and I thought it might
be something we could utilize.  Supposedly it returns PIDs for processes
having that file open, but how this works for pipe read/write sides
needs testing.  Of course, knowing how Windows functions usually only go
half the way, the function will either only return the current side of
the pipe, or even an error code :-/  Never mind, it was just an idea.


Corinna

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-11 11:33                             ` Corinna Vinschen
@ 2021-11-11 12:02                               ` Takashi Yano
  2021-11-11 13:20                                 ` Takashi Yano
  0 siblings, 1 reply; 33+ messages in thread
From: Takashi Yano @ 2021-11-11 12:02 UTC (permalink / raw)
  To: cygwin-developers

On Thu, 11 Nov 2021 12:33:21 +0100
Corinna Vinschen wrote:
> On Nov 11 20:12, Takashi Yano wrote:
> > > [FileProcessIdsUsingFileInformation]
> > 
> > Thanks for advice. get_query_hdl_per_* is called in the write side,
> > so only knows pipe handle of the write side. We would like to know
> > ProcessId which have pipe handle of the read side. Can we use
> > FileProcessIdsUsingFileInformation for this perpose?
> 
> I don't know.  I just stumbled over it yesterday and I thought it might
> be something we could utilize.  Supposedly it returns PIDs for processes
> having that file open, but how this works for pipe read/write sides
> needs testing.  Of course, knowing how Windows functions usually only go
> half the way, the function will either only return the current side of
> the pipe, or even an error code :-/  Never mind, it was just an idea.

I have tested the behaviour of FileProcessIdsUsingFileInformation
just now. We can use it! I will try to use it in get_query_hdl().

Thanks!

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

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-11 12:02                               ` Takashi Yano
@ 2021-11-11 13:20                                 ` Takashi Yano
  2021-11-11 16:07                                   ` Corinna Vinschen
  0 siblings, 1 reply; 33+ messages in thread
From: Takashi Yano @ 2021-11-11 13:20 UTC (permalink / raw)
  To: cygwin-developers

[-- Attachment #1: Type: text/plain, Size: 1843 bytes --]

On Thu, 11 Nov 2021 21:02:34 +0900
Takashi Yano wrote:
> On Thu, 11 Nov 2021 12:33:21 +0100
> Corinna Vinschen wrote:
> > On Nov 11 20:12, Takashi Yano wrote:
> > > > [FileProcessIdsUsingFileInformation]
> > > 
> > > Thanks for advice. get_query_hdl_per_* is called in the write side,
> > > so only knows pipe handle of the write side. We would like to know
> > > ProcessId which have pipe handle of the read side. Can we use
> > > FileProcessIdsUsingFileInformation for this perpose?
> > 
> > I don't know.  I just stumbled over it yesterday and I thought it might
> > be something we could utilize.  Supposedly it returns PIDs for processes
> > having that file open, but how this works for pipe read/write sides
> > needs testing.  Of course, knowing how Windows functions usually only go
> > half the way, the function will either only return the current side of
> > the pipe, or even an error code :-/  Never mind, it was just an idea.
> 
> I have tested the behaviour of FileProcessIdsUsingFileInformation
> just now. We can use it! I will try to use it in get_query_hdl().

I have tried to utilize FileProcessIdsUsingFileInformation in
get_query_hdl_per_process() as the patch attached. It works as
expected.

I also measured the response of select() using these functions.
The first time and second time responses are measured. The second
time should be much faster than the first time because search
result has been cached.

First time, Second time
 4.620400 [msec], 0.102300 [msec] << get_query_hdl_per_process()
19.080400 [msec], 0.199000 [msec] << get_query_hdl_per_system()
14.364300 [msec], 0.156800 [msec] << FileProcessIdsUsingFileInformation

Unfortunately, FileProcessIdsUsingFileInformation is slower than
current get_query_hdl_per_process(). It takes about 3 times longer
time.

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

[-- Attachment #2: fpiufi.patch --]
[-- Type: application/octet-stream, Size: 2977 bytes --]

diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
index 9392a28c1..83c09d7c6 100644
--- a/winsup/cygwin/fhandler_pipe.cc
+++ b/winsup/cygwin/fhandler_pipe.cc
@@ -1201,50 +1201,32 @@ fhandler_pipe::get_query_hdl_per_process (WCHAR *name,
   NTSTATUS status;
   ULONG len;
   DWORD n_process = 256;
-  PSYSTEM_PROCESS_INFORMATION spi;
+  FILE_PROCESS_IDS_USING_FILE_INFORMATION *fpiufi;
   do
-    { /* Enumerate processes */
-      DWORD nbytes = n_process * sizeof (SYSTEM_PROCESS_INFORMATION);
-      spi = (PSYSTEM_PROCESS_INFORMATION) HeapAlloc (GetProcessHeap (),
-						     0, nbytes);
-      if (!spi)
+    { /* Enumerate processes having the pipe handle. */
+      IO_STATUS_BLOCK io;
+      DWORD nbytes = n_process * sizeof (ULONG)
+	+ sizeof (FILE_PROCESS_IDS_USING_FILE_INFORMATION);
+      fpiufi = (FILE_PROCESS_IDS_USING_FILE_INFORMATION *)
+	HeapAlloc (GetProcessHeap (), 0, nbytes);
+      if (!fpiufi)
 	return NULL;
-      status = NtQuerySystemInformation (SystemProcessInformation,
-					 spi, nbytes, &len);
+      status = NtQueryInformationFile (get_handle (), &io, fpiufi, nbytes,
+				       FileProcessIdsUsingFileInformation);
       if (NT_SUCCESS (status))
 	break;
-      HeapFree (GetProcessHeap (), 0, spi);
+      HeapFree (GetProcessHeap (), 0, fpiufi);
       n_process *= 2;
     }
   while (n_process < (1L<<20) && status == STATUS_INFO_LENGTH_MISMATCH);
   if (!NT_SUCCESS (status))
     return NULL;
 
-  /* In most cases, it is faster to check the processes in reverse order.
-     To do this, store PIDs into an array. */
-  DWORD *proc_pids = (DWORD *) HeapAlloc (GetProcessHeap (), 0,
-					  n_process * sizeof (DWORD));
-  if (!proc_pids)
-    {
-      HeapFree (GetProcessHeap (), 0, spi);
-      return NULL;
-    }
-  PSYSTEM_PROCESS_INFORMATION p = spi;
-  n_process = 0;
-  while (true)
-    {
-      proc_pids[n_process++] = (DWORD)(intptr_t) p->UniqueProcessId;
-      if (!p->NextEntryOffset)
-	break;
-      p = (PSYSTEM_PROCESS_INFORMATION) ((char *) p + p->NextEntryOffset);
-    }
-  HeapFree (GetProcessHeap (), 0, spi);
-
-  for (LONG i = (LONG) n_process - 1; i >= 0; i--)
+  for (ULONG i = 0; i < fpiufi->NumberOfProcessIdsInList; i++)
     {
       HANDLE proc = OpenProcess (PROCESS_DUP_HANDLE
 				 | PROCESS_QUERY_INFORMATION,
-				 0, proc_pids[i]);
+				 0, fpiufi->ProcessIdList[i]);
       if (!proc)
 	continue;
 
@@ -1298,7 +1280,7 @@ fhandler_pipe::get_query_hdl_per_process (WCHAR *name,
 	      query_hdl_proc = proc;
 	      query_hdl_value = (HANDLE)(intptr_t) phi->Handles[j].HandleValue;
 	      HeapFree (GetProcessHeap (), 0, phi);
-	      HeapFree (GetProcessHeap (), 0, proc_pids);
+	      HeapFree (GetProcessHeap (), 0, fpiufi);
 	      return h;
 	    }
 close_handle:
@@ -1308,7 +1290,7 @@ close_handle:
 close_proc:
       CloseHandle (proc);
     }
-  HeapFree (GetProcessHeap (), 0, proc_pids);
+  HeapFree (GetProcessHeap (), 0, fpiufi);
   return NULL;
 }
 

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-11 13:20                                 ` Takashi Yano
@ 2021-11-11 16:07                                   ` Corinna Vinschen
  0 siblings, 0 replies; 33+ messages in thread
From: Corinna Vinschen @ 2021-11-11 16:07 UTC (permalink / raw)
  To: cygwin-developers

On Nov 11 22:20, Takashi Yano wrote:
> On Thu, 11 Nov 2021 21:02:34 +0900
> Takashi Yano wrote:
> > On Thu, 11 Nov 2021 12:33:21 +0100
> > Corinna Vinschen wrote:
> > > On Nov 11 20:12, Takashi Yano wrote:
> > > > > [FileProcessIdsUsingFileInformation]
> > > > 
> > > > Thanks for advice. get_query_hdl_per_* is called in the write side,
> > > > so only knows pipe handle of the write side. We would like to know
> > > > ProcessId which have pipe handle of the read side. Can we use
> > > > FileProcessIdsUsingFileInformation for this perpose?
> > > 
> > > I don't know.  I just stumbled over it yesterday and I thought it might
> > > be something we could utilize.  Supposedly it returns PIDs for processes
> > > having that file open, but how this works for pipe read/write sides
> > > needs testing.  Of course, knowing how Windows functions usually only go
> > > half the way, the function will either only return the current side of
> > > the pipe, or even an error code :-/  Never mind, it was just an idea.
> > 
> > I have tested the behaviour of FileProcessIdsUsingFileInformation
> > just now. We can use it! I will try to use it in get_query_hdl().
> 
> I have tried to utilize FileProcessIdsUsingFileInformation in
> get_query_hdl_per_process() as the patch attached. It works as
> expected.
> 
> I also measured the response of select() using these functions.
> The first time and second time responses are measured. The second
> time should be much faster than the first time because search
> result has been cached.
> 
> First time, Second time
>  4.620400 [msec], 0.102300 [msec] << get_query_hdl_per_process()
> 19.080400 [msec], 0.199000 [msec] << get_query_hdl_per_system()
> 14.364300 [msec], 0.156800 [msec] << FileProcessIdsUsingFileInformation
> 
> Unfortunately, FileProcessIdsUsingFileInformation is slower than
> current get_query_hdl_per_process(). It takes about 3 times longer
> time.

*laughing manically*

Your previous mail was a nice surprise, but now we're back on earth it
seems.

On second thought I guess this even makes sense.  At Vista times
NtQueryInformationProcess(ProcessHandleInformation) didn't exist,
so the method to check this is probably along the lines of your
get_query_hdl_per_system function, just faster because it's all
implemented in a single system call.  And it's probably still
implemented this way.

Oh well.  Thanks for testing!


Corinna

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-10 21:32                         ` Ken Brown
@ 2021-11-11 16:11                           ` Ken Brown
  2021-11-12  8:38                             ` Takashi Yano
  2021-11-16 23:46                             ` Takashi Yano
  0 siblings, 2 replies; 33+ messages in thread
From: Ken Brown @ 2021-11-11 16:11 UTC (permalink / raw)
  To: cygwin-developers

On 11/10/2021 4:32 PM, Ken Brown wrote:
> On 11/10/2021 3:35 PM, Corinna Vinschen wrote:
>> On Nov 10 22:30, Takashi Yano wrote:
>>> Thanks. I have just pushed the experimental patch to topic/pipe.
>>> Please try. If something wrong, please point it out.
>>
>> Great, I'll have a look.  Ken, you're looking as well, right?
> 
> Yes.  It looks good on a first read-through.  I want to look again and do some 
> testing.  One minor thing I noticed is that fhandler_pipe::raw_read could be 
> cleaned up a little more: There are still references to STATUS_THREAD_SIGNALED 
> and STATUS_THREAD_CANCELED, which can't occur any more.

It still looks good after a second read.  I'll try using it in my normal Cygwin 
install to make sure no problems come up.

I've pushed a cleanup patch removing STATUS_THREAD_SIGNALED and 
STATUS_THREAD_CANCELED, as well as a patch documenting the new behavior of the 
pipe_byte option.

Ken

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-11 11:12                           ` Takashi Yano
  2021-11-11 11:33                             ` Corinna Vinschen
@ 2021-11-12  8:33                             ` Takashi Yano
  2021-11-12 10:02                               ` Corinna Vinschen
  1 sibling, 1 reply; 33+ messages in thread
From: Takashi Yano @ 2021-11-12  8:33 UTC (permalink / raw)
  To: cygwin-developers

On Thu, 11 Nov 2021 20:12:06 +0900
Takashi Yano wrote:
> On Thu, 11 Nov 2021 10:52:33 +0100
> Corinna Vinschen wrote:
> > What I don't like much is that we are now running a busy loop, but the
> > NtQueryObject hang is... disappointing, to say the least.  What about
> > using another function like NtQueryInformationFile(FileNameInformation)?
> > I bet this hangs, too, right?  
> 
> I have just confirmed that NtQueryInformationFile(FileNameInformation)
> also hangs.

I tried to utilize following APIs, however, all of them hangs.

NtQueryObject(ObjectNameInformation)
NtQueryInformationFile(FileNameInformation)
GetFinalPathNameByHandle()
GetFileInformationByHandleEx(FileNameInfo)

However, NtQueryInformationFile(FileProcessIdsUsingFileInformation)
does not hang. I wonder how it checks internally that one of the
process handles is pointing the same pipe.

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

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-11 16:11                           ` Ken Brown
@ 2021-11-12  8:38                             ` Takashi Yano
  2021-11-16 23:46                             ` Takashi Yano
  1 sibling, 0 replies; 33+ messages in thread
From: Takashi Yano @ 2021-11-12  8:38 UTC (permalink / raw)
  To: cygwin-developers

On Thu, 11 Nov 2021 11:11:10 -0500
Ken Brown wrote:
> On 11/10/2021 4:32 PM, Ken Brown wrote:
> > On 11/10/2021 3:35 PM, Corinna Vinschen wrote:
> >> On Nov 10 22:30, Takashi Yano wrote:
> >>> Thanks. I have just pushed the experimental patch to topic/pipe.
> >>> Please try. If something wrong, please point it out.
> >>
> >> Great, I'll have a look.  Ken, you're looking as well, right?
> > 
> > Yes.  It looks good on a first read-through.  I want to look again and do some 
> > testing.  One minor thing I noticed is that fhandler_pipe::raw_read could be 
> > cleaned up a little more: There are still references to STATUS_THREAD_SIGNALED 
> > and STATUS_THREAD_CANCELED, which can't occur any more.
> 
> It still looks good after a second read.  I'll try using it in my normal Cygwin 
> install to make sure no problems come up.
> 
> I've pushed a cleanup patch removing STATUS_THREAD_SIGNALED and 
> STATUS_THREAD_CANCELED, as well as a patch documenting the new behavior of the 
> pipe_byte option.

Thanks for reviewing and cleaning up the code, as well as
document changes!

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

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-12  8:33                             ` Takashi Yano
@ 2021-11-12 10:02                               ` Corinna Vinschen
  2021-12-12 13:26                                 ` Takashi Yano
  0 siblings, 1 reply; 33+ messages in thread
From: Corinna Vinschen @ 2021-11-12 10:02 UTC (permalink / raw)
  To: cygwin-developers

On Nov 12 17:33, Takashi Yano wrote:
> On Thu, 11 Nov 2021 20:12:06 +0900
> Takashi Yano wrote:
> > On Thu, 11 Nov 2021 10:52:33 +0100
> > Corinna Vinschen wrote:
> > > What I don't like much is that we are now running a busy loop, but the
> > > NtQueryObject hang is... disappointing, to say the least.  What about
> > > using another function like NtQueryInformationFile(FileNameInformation)?
> > > I bet this hangs, too, right?  
> > 
> > I have just confirmed that NtQueryInformationFile(FileNameInformation)
> > also hangs.
> 
> I tried to utilize following APIs, however, all of them hangs.
> 
> NtQueryObject(ObjectNameInformation)
> NtQueryInformationFile(FileNameInformation)
> GetFinalPathNameByHandle()
> GetFileInformationByHandleEx(FileNameInfo)

:-P

> However, NtQueryInformationFile(FileProcessIdsUsingFileInformation)
> does not hang. I wonder how it checks internally that one of the
> process handles is pointing the same pipe.

Good question.

In the meantime, you might want to merge your code into master, whenever
you think it's ready.


Corinna

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-11 16:11                           ` Ken Brown
  2021-11-12  8:38                             ` Takashi Yano
@ 2021-11-16 23:46                             ` Takashi Yano
  2021-11-17  8:10                               ` Takashi Yano
  1 sibling, 1 reply; 33+ messages in thread
From: Takashi Yano @ 2021-11-16 23:46 UTC (permalink / raw)
  To: cygwin-developers

[-- Attachment #1: Type: text/plain, Size: 1187 bytes --]

On Thu, 11 Nov 2021 11:11:10 -0500
Ken Brown wrote:
> On 11/10/2021 4:32 PM, Ken Brown wrote:
> > On 11/10/2021 3:35 PM, Corinna Vinschen wrote:
> >> On Nov 10 22:30, Takashi Yano wrote:
> >>> Thanks. I have just pushed the experimental patch to topic/pipe.
> >>> Please try. If something wrong, please point it out.
> >>
> >> Great, I'll have a look.  Ken, you're looking as well, right?
> > 
> > Yes.  It looks good on a first read-through.  I want to look again and do some 
> > testing.  One minor thing I noticed is that fhandler_pipe::raw_read could be 
> > cleaned up a little more: There are still references to STATUS_THREAD_SIGNALED 
> > and STATUS_THREAD_CANCELED, which can't occur any more.
> 
> It still looks good after a second read.  I'll try using it in my normal Cygwin 
> install to make sure no problems come up.
> 
> I've pushed a cleanup patch removing STATUS_THREAD_SIGNALED and 
> STATUS_THREAD_CANCELED, as well as a patch documenting the new behavior of the 
> pipe_byte option.

I have just rebased topic/pipe to current master.

I also would like to propose patch attached for topic/pipe.
Please have a look.

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

[-- Attachment #2: 0001-Cygwin-pipe-Suppress-unnecessary-set_pipe_non_blocki.patch --]
[-- Type: application/octet-stream, Size: 2290 bytes --]

From 544292f81d8914b8510f39fe07d2b05cd4e99828 Mon Sep 17 00:00:00 2001
From: "U-EXPRESS5800-S70\\yano" <takashi.yano@nifty.ne.jp>
Date: Tue, 16 Nov 2021 20:59:09 +0900
Subject: [PATCH] Cygwin: pipe: Suppress unnecessary set_pipe_non_blocking()
 call.

- Call set_pipe_non_blocking(false) only if the pipe will be really
  inherited to non-cygwin process.
- Set blocking mode properly when cygwin process is executed from non-
  cygwin process.
---
 winsup/cygwin/dtable.cc | 3 +++
 winsup/cygwin/spawn.cc  | 9 ++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/winsup/cygwin/dtable.cc b/winsup/cygwin/dtable.cc
index d57cbb355..e54db4446 100644
--- a/winsup/cygwin/dtable.cc
+++ b/winsup/cygwin/dtable.cc
@@ -410,6 +410,9 @@ dtable::init_std_file_from_handle (int fd, HANDLE handle)
 	{
 	  fhandler_pipe *fhp = (fhandler_pipe *) fh;
 	  fhp->set_pipe_buf_size ();
+	  /* Set read pipe always to nonblocking */
+	  fhp->set_pipe_non_blocking (fhp->get_device () == FH_PIPER ?
+				      true : fhp->is_nonblocking ());
 	}
 
       if (!fh->open_setup (openflags))
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 6b2026776..e160fa3bb 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -648,8 +648,9 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
 
       if (!iscygwin ())
 	{
+	  int fd;
 	  cfd.rewind ();
-	  while (cfd.next () >= 0)
+	  while ((fd = cfd.next ()) >= 0)
 	    if (cfd->get_major () == DEV_PTYS_MAJOR)
 	      {
 		fhandler_pty_slave *ptys =
@@ -657,13 +658,15 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
 		ptys->create_invisible_console ();
 		ptys->setup_locale ();
 	      }
-	    else if (cfd->get_dev () == FH_PIPEW)
+	    else if (cfd->get_dev () == FH_PIPEW
+		     && (fd == (in__stdout < 0 ? 1 : in__stdout) || fd == 2))
 	      {
 		fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
 		pipe->close_query_handle ();
 		pipe->set_pipe_non_blocking (false);
 	      }
-	    else if (cfd->get_dev () == FH_PIPER)
+	    else if (cfd->get_dev () == FH_PIPER
+		     && fd == (in__stdin < 0 ? 0 : in__stdin))
 	      {
 		fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
 		pipe->set_pipe_non_blocking (false);
-- 
2.33.0


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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-16 23:46                             ` Takashi Yano
@ 2021-11-17  8:10                               ` Takashi Yano
  2021-11-17 15:12                                 ` Ken Brown
  0 siblings, 1 reply; 33+ messages in thread
From: Takashi Yano @ 2021-11-17  8:10 UTC (permalink / raw)
  To: cygwin-developers

[-- Attachment #1: Type: text/plain, Size: 1557 bytes --]

On Wed, 17 Nov 2021 08:46:50 +0900
Takashi Yano wrote:
> On Thu, 11 Nov 2021 11:11:10 -0500
> Ken Brown wrote:
> > On 11/10/2021 4:32 PM, Ken Brown wrote:
> > > On 11/10/2021 3:35 PM, Corinna Vinschen wrote:
> > >> On Nov 10 22:30, Takashi Yano wrote:
> > >>> Thanks. I have just pushed the experimental patch to topic/pipe.
> > >>> Please try. If something wrong, please point it out.
> > >>
> > >> Great, I'll have a look.  Ken, you're looking as well, right?
> > > 
> > > Yes.  It looks good on a first read-through.  I want to look again and do some 
> > > testing.  One minor thing I noticed is that fhandler_pipe::raw_read could be 
> > > cleaned up a little more: There are still references to STATUS_THREAD_SIGNALED 
> > > and STATUS_THREAD_CANCELED, which can't occur any more.
> > 
> > It still looks good after a second read.  I'll try using it in my normal Cygwin 
> > install to make sure no problems come up.
> > 
> > I've pushed a cleanup patch removing STATUS_THREAD_SIGNALED and 
> > STATUS_THREAD_CANCELED, as well as a patch documenting the new behavior of the 
> > pipe_byte option.
> 
> I have just rebased topic/pipe to current master.
> 
> I also would like to propose patch attached for topic/pipe.
> Please have a look.

Ah, the spawn.cc part of this patch should be applied not
only topic/pipe but also master and cygwin-3_3-branch.
I have resubmitted that to cygwin-patches mailing list.

Only the dtable.cc part of this patch is for topic/pipe.
Please have a look attached patch.

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

[-- Attachment #2: 0001-Cygwin-pipe-Restore-blocking-mode-for-cygwin-process.patch --]
[-- Type: application/octet-stream, Size: 976 bytes --]

From 142c87193b6074e2cee8bd4b3a78b33ee7d705e8 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Wed, 17 Nov 2021 13:08:05 +0900
Subject: [PATCH] Cygwin: pipe: Restore blocking mode for cygwin process at
 startup.

- Set blocking mode properly at startup of cygwin process. This is
  needed if the cygwin process is executed from non-cygwin process.
---
 winsup/cygwin/dtable.cc | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/winsup/cygwin/dtable.cc b/winsup/cygwin/dtable.cc
index d57cbb355..e54db4446 100644
--- a/winsup/cygwin/dtable.cc
+++ b/winsup/cygwin/dtable.cc
@@ -410,6 +410,9 @@ dtable::init_std_file_from_handle (int fd, HANDLE handle)
 	{
 	  fhandler_pipe *fhp = (fhandler_pipe *) fh;
 	  fhp->set_pipe_buf_size ();
+	  /* Set read pipe always to nonblocking */
+	  fhp->set_pipe_non_blocking (fhp->get_device () == FH_PIPER ?
+				      true : fhp->is_nonblocking ());
 	}
 
       if (!fh->open_setup (openflags))
-- 
2.33.0


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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-17  8:10                               ` Takashi Yano
@ 2021-11-17 15:12                                 ` Ken Brown
  0 siblings, 0 replies; 33+ messages in thread
From: Ken Brown @ 2021-11-17 15:12 UTC (permalink / raw)
  To: cygwin-developers

On 11/17/2021 3:10 AM, Takashi Yano wrote:
> On Wed, 17 Nov 2021 08:46:50 +0900
> Takashi Yano wrote:
>> I also would like to propose patch attached for topic/pipe.
>> Please have a look.
> 
> Ah, the spawn.cc part of this patch should be applied not
> only topic/pipe but also master and cygwin-3_3-branch.
> I have resubmitted that to cygwin-patches mailing list.
> 
> Only the dtable.cc part of this patch is for topic/pipe.
> Please have a look attached patch.

LGTM.

Ken

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-11-12 10:02                               ` Corinna Vinschen
@ 2021-12-12 13:26                                 ` Takashi Yano
  2021-12-12 13:36                                   ` Ken Brown
  0 siblings, 1 reply; 33+ messages in thread
From: Takashi Yano @ 2021-12-12 13:26 UTC (permalink / raw)
  To: cygwin-developers

On Fri, 12 Nov 2021 11:02:46 +0100
Corinna Vinschen wrote:
> In the meantime, you might want to merge your code into master, whenever
> you think it's ready.

I have rebased the topic/pipe branch to current master.

In addition, I think we had already enough test period for
new code in topic/pipe. What about merging it into master?

Or does anyone have any concerns?

BTW, how can we merge the topic/pipe branch? Using "git merge"?
Or rebase master to topic/pipe branch, i.e. "git rebase topic/pipe"?

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

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-12-12 13:26                                 ` Takashi Yano
@ 2021-12-12 13:36                                   ` Ken Brown
  2021-12-13 11:15                                     ` Takashi Yano
  0 siblings, 1 reply; 33+ messages in thread
From: Ken Brown @ 2021-12-12 13:36 UTC (permalink / raw)
  To: cygwin-developers

On 12/12/2021 8:26 AM, Takashi Yano wrote:
> On Fri, 12 Nov 2021 11:02:46 +0100
> Corinna Vinschen wrote:
>> In the meantime, you might want to merge your code into master, whenever
>> you think it's ready.
> 
> I have rebased the topic/pipe branch to current master.
> 
> In addition, I think we had already enough test period for
> new code in topic/pipe. What about merging it into master?
> 
> Or does anyone have any concerns?

I think we're ready.

> BTW, how can we merge the topic/pipe branch? Using "git merge"?
> Or rebase master to topic/pipe branch, i.e. "git rebase topic/pipe"?

Since you've already rebased topic/pipe onto master, I think you can just 
checkout master and do "git merge topic/pipe".  It should just be a simple 
fast-forward merge.

Ken

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

* Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot
  2021-12-12 13:36                                   ` Ken Brown
@ 2021-12-13 11:15                                     ` Takashi Yano
  0 siblings, 0 replies; 33+ messages in thread
From: Takashi Yano @ 2021-12-13 11:15 UTC (permalink / raw)
  To: cygwin-developers

On Sun, 12 Dec 2021 08:36:13 -0500
Ken Brown wrote:
> On 12/12/2021 8:26 AM, Takashi Yano wrote:
> > On Fri, 12 Nov 2021 11:02:46 +0100
> > Corinna Vinschen wrote:
> >> In the meantime, you might want to merge your code into master, whenever
> >> you think it's ready.
> > 
> > I have rebased the topic/pipe branch to current master.
> > 
> > In addition, I think we had already enough test period for
> > new code in topic/pipe. What about merging it into master?
> > 
> > Or does anyone have any concerns?
> 
> I think we're ready.
> 
> > BTW, how can we merge the topic/pipe branch? Using "git merge"?
> > Or rebase master to topic/pipe branch, i.e. "git rebase topic/pipe"?
> 
> Since you've already rebased topic/pipe onto master, I think you can just 
> checkout master and do "git merge topic/pipe".  It should just be a simple 
> fast-forward merge.

Thanks. Merged.

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

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

end of thread, other threads:[~2021-12-13 11:15 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CAEv6GOB8PXHgHoz7hdJy6Bia2GWEmUDnTd252gTGinz2vuv=hA@mail.gmail.com>
     [not found] ` <20211105123950.b118a7f2ba38379764df4c12@nifty.ne.jp>
     [not found]   ` <CAEv6GOA-y58YrftXgEgFrjqtOTHmfdu2Vrq76Lwn0suZpZ=U9w@mail.gmail.com>
     [not found]     ` <20211105170542.96ce6dd4ca32880ddfddd660@nifty.ne.jp>
     [not found]       ` <CAEv6GODiM88Xfhk9R3AcEW6UTYSzACzYe4C0gPoTYm=u9ZTqRQ@mail.gmail.com>
     [not found]         ` <20211106044116.698b465a5d8ed6ce2cc75c99@nifty.ne.jp>
     [not found]           ` <2cfa5de7-3b95-9062-4572-f36d304bc916@cornell.edu>
2021-11-06  6:10             ` 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot Takashi Yano
2021-11-06 11:42               ` Corinna Vinschen
2021-11-06 12:02                 ` Corinna Vinschen
2021-11-06 14:13                   ` Takashi Yano
2021-11-06 17:20                     ` Corinna Vinschen
2021-11-07  3:01                       ` Takashi Yano
2021-11-06 16:38                 ` Ken Brown
2021-11-06 17:20                   ` Corinna Vinschen
2021-11-07  3:46                   ` Takashi Yano
2021-11-07 22:20                     ` Ken Brown
2021-11-08  8:23                       ` Takashi Yano
2021-11-08  9:46                         ` Corinna Vinschen
2021-11-10  8:30                 ` Takashi Yano
2021-11-10 10:34                   ` Corinna Vinschen
2021-11-10 13:30                     ` Takashi Yano
2021-11-10 20:35                       ` Corinna Vinschen
2021-11-10 21:32                         ` Ken Brown
2021-11-11 16:11                           ` Ken Brown
2021-11-12  8:38                             ` Takashi Yano
2021-11-16 23:46                             ` Takashi Yano
2021-11-17  8:10                               ` Takashi Yano
2021-11-17 15:12                                 ` Ken Brown
2021-11-11  9:52                         ` Corinna Vinschen
2021-11-11 11:12                           ` Takashi Yano
2021-11-11 11:33                             ` Corinna Vinschen
2021-11-11 12:02                               ` Takashi Yano
2021-11-11 13:20                                 ` Takashi Yano
2021-11-11 16:07                                   ` Corinna Vinschen
2021-11-12  8:33                             ` Takashi Yano
2021-11-12 10:02                               ` Corinna Vinschen
2021-12-12 13:26                                 ` Takashi Yano
2021-12-12 13:36                                   ` Ken Brown
2021-12-13 11:15                                     ` 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).