cygwin-2 process handling

cygwin-2 process handling

[Michael Mauger]

Since the upgrade to cygwin-2 I've been having consistent problems using emacs.  My emacs config has not changed and the most recent update to cygwin-2 has helped.  I'm using the emacs-w32 executable and have numerous remote cygwin ssh sessions running underneath emacs so that I can edit remote files and run remote shell sessions within emacs.  

When I then start up a Windows console executable (in my case Oracles sqlplus.exe, but cmd.exe works too), I start getting hanging in the cygwin ssh sessions.  Filename completion (which involves emacs interacting with a ssh session) or opening a remote file (which involves emacs transferring the file locally via scp or inline) will both hang.  It generally requires that I send a SIGUSR2 signal to the emacs-w32 process which emacs responds to by halting whatever is running and returning to the top level interaction loop.  When it does so, the process appears to be waiting for input. If I then end the console session, the interaction returns to normal.  Trying to start a second Windows console session will also hang.

In Emacs *scratch* buffer
  (shell "*One*")  C-j
  #<buffer *One*>
  (shell "*Two*")  C-j
  #<buffer *Two*>


  (let ((explicit-shell-file-name "/c/Windows/System32/cmd.exe")) (shell "*Three*"))  C-j  

  #<buffer *Three*>  (let ((explicit-shell-file-name "/c/Windows/System32/cmd.exe")) (shell "*Four*"))  C-j
  *** HANGS ***



Things dramatically improved with Cygwin-2.0.2-1 "restores the #include <sys/select.h> in <sys/time.h>".  I could run more than one process at a time, and worked perfectly prior to cygwin-2.


MICHAEL MAUGER // FSF Member // GNU Emacs sql-mode maintainer // GNU Linux, GNU Emacs, OwnCloud

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: cygwin-2 process handling

[Ken Brown]

On 5/18/2015 1:15 PM, Michael Mauger wrote:
> Since the upgrade to cygwin-2 I've been having consistent problems using emacs.  My emacs config has not changed and the most recent update to cygwin-2 has helped.  I'm using the emacs-w32 executable and have numerous remote cygwin ssh sessions running underneath emacs so that I can edit remote files and run remote shell sessions within emacs.
>
> When I then start up a Windows console executable (in my case Oracles sqlplus.exe, but cmd.exe works too), I start getting hanging in the cygwin ssh sessions.  Filename completion (which involves emacs interacting with a ssh session) or opening a remote file (which involves emacs transferring the file locally via scp or inline) will both hang.  It generally requires that I send a SIGUSR2 signal to the emacs-w32 process which emacs responds to by halting whatever is running and returning to the top level interaction loop.  When it does so, the process appears to be waiting for input. If I then end the console session, the interaction returns to normal.  Trying to start a second Windows console session will also hang.
>
> In Emacs *scratch* buffer
>    (shell "*One*")  C-j
>    #<buffer *One*>
>    (shell "*Two*")  C-j
>    #<buffer *Two*>
>
>
>    (let ((explicit-shell-file-name "/c/Windows/System32/cmd.exe")) (shell "*Three*"))  C-j
>
>    #<buffer *Three*>  (let ((explicit-shell-file-name "/c/Windows/System32/cmd.exe")) (shell "*Four*"))  C-j
>    *** HANGS ***

I can reproduce this, and even more simply.  If I start a single shell 
running cmd.exe, then emacs hangs on the next attempt to call a 
subprocess.  For example, I first evaluate the following in the 
*scratch* buffer

(let ((explicit-shell-file-name "/c/Windows/System32/cmd.exe")) (shell))

I then try to list a directory with 'C-x d', and emacs hangs.

The problem appears in the 2015-03-17 cygwin snapshot but not in the 
2015-03-11 snapshot.  I'm trying to do a git bisection to narrow it down 
further, but so far I haven't succeeded because I get errors when I try 
to build the git checkouts from around that time.  I'll keep trying.

Ken

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: cygwin-2 process handling

[Ken Brown]

On 5/19/2015 1:14 PM, Ken Brown wrote:
> On 5/18/2015 1:15 PM, Michael Mauger wrote:
>> Since the upgrade to cygwin-2 I've been having consistent problems
>> using emacs.  My emacs config has not changed and the most recent
>> update to cygwin-2 has helped.  I'm using the emacs-w32 executable and
>> have numerous remote cygwin ssh sessions running underneath emacs so
>> that I can edit remote files and run remote shell sessions within emacs.
>>
>> When I then start up a Windows console executable (in my case Oracles
>> sqlplus.exe, but cmd.exe works too), I start getting hanging in the
>> cygwin ssh sessions.  Filename completion (which involves emacs
>> interacting with a ssh session) or opening a remote file (which
>> involves emacs transferring the file locally via scp or inline) will
>> both hang.  It generally requires that I send a SIGUSR2 signal to the
>> emacs-w32 process which emacs responds to by halting whatever is
>> running and returning to the top level interaction loop.  When it does
>> so, the process appears to be waiting for input. If I then end the
>> console session, the interaction returns to normal.  Trying to start a
>> second Windows console session will also hang.
>>
>> In Emacs *scratch* buffer
>>    (shell "*One*")  C-j
>>    #<buffer *One*>
>>    (shell "*Two*")  C-j
>>    #<buffer *Two*>
>>
>>
>>    (let ((explicit-shell-file-name "/c/Windows/System32/cmd.exe"))
>> (shell "*Three*"))  C-j
>>
>>    #<buffer *Three*>  (let ((explicit-shell-file-name
>> "/c/Windows/System32/cmd.exe")) (shell "*Four*"))  C-j
>>    *** HANGS ***
>
> I can reproduce this, and even more simply.  If I start a single shell
> running cmd.exe, then emacs hangs on the next attempt to call a
> subprocess.  For example, I first evaluate the following in the
> *scratch* buffer
>
> (let ((explicit-shell-file-name "/c/Windows/System32/cmd.exe")) (shell))
>
> I then try to list a directory with 'C-x d', and emacs hangs.
>
> The problem appears in the 2015-03-17 cygwin snapshot but not in the
> 2015-03-11 snapshot.  I'm trying to do a git bisection to narrow it down
> further, but so far I haven't succeeded because I get errors when I try
> to build the git checkouts from around that time.  I'll keep trying.

OK, I got the bisection to work:

32fd5b0fe1548cff79b9868e4e259853c59a44db is the first bad commit
commit 32fd5b0fe1548cff79b9868e4e259853c59a44db
Author: Takashi Yano <xxx>
Date:   Thu Mar 12 15:48:10 2015 +0100

     Let pty slave detect closure of last master handle

         * fhandler_tty.cc (fhandler_pty_master::close): Add code to 
make slave
         detect closure of master. Fix typo in error message.


Takashi, can you help?  If not, we'll have to wait for Corinna to get 
back from vacation.

Ken


--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: cygwin-2 process handling

[Takashi Yano]

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

Hi Ken,

On Tue, 19 May 2015 14:16:35 -0400
Ken Brown wrote:

> OK, I got the bisection to work:
> 
> 32fd5b0fe1548cff79b9868e4e259853c59a44db is the first bad commit
> commit 32fd5b0fe1548cff79b9868e4e259853c59a44db
> Author: Takashi Yano <xxx>
> Date:   Thu Mar 12 15:48:10 2015 +0100
> 
>      Let pty slave detect closure of last master handle
> 
>          * fhandler_tty.cc (fhandler_pty_master::close): Add code to 
> make slave
>          detect closure of master. Fix typo in error message.
> 
> 
> Takashi, can you help?  If not, we'll have to wait for Corinna to get 
> back from vacation.

I was looking into this problem, and found the PeekNamedPipe()
call is blocked in fhandler_pty_master::close() when the problem
occurs.

I had not noticed that, 

https://msdn.microsoft.com/en-us/library/windows/desktop/aa365779(v=vs.85).aspx

says:
> The PeekNamedPipe function can block thread execution the same
> way any I/O function can when called on a synchronous handle in
> a multi-threaded application.

I have made a patch attached to resolve this problem. Please have
a look.

With this patch, NtQueryObject() is used instead of PeekNamedPipe()
to detect closure of the last master handle.

Please refer to the following discussion for more details.
http://cygwin.com/ml/cygwin/2015-03/msg00221.html


ChageLog is as follows.

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

	* fhandler_tty.cc (fhandler_pty_master::close): Use NtQueryObject()
	instead of PeekNamedPipe() to detect closing the last master handle.

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

[-- Attachment #2: cygwin.patch.20150521 --]
[-- Type: application/octet-stream, Size: 1543 bytes --]

diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index e91b3e3..95bf445 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -1309,13 +1309,37 @@ fhandler_pty_master::close ()
 	}
     }
 
+  HANDLE output_handle_local;
+  if (!DuplicateHandle (GetCurrentProcess (), get_output_handle (),
+		       GetCurrentProcess (), &output_handle_local,
+		       0, TRUE, DUPLICATE_SAME_ACCESS))
+    {
+      termios_printf ("DuplicateHandle (output_handle), %E");
+      output_handle_local = NULL;
+      SetEvent (input_available_event);
+    }
+
   fhandler_pty_common::close ();
 
   /* Check if the last master handle has been closed.  If so, set
      input_available_event to wake up potentially waiting slaves. */
-  if (!PeekNamedPipe (from_master, NULL, 0, NULL, NULL, NULL)
-      && GetLastError () == ERROR_BROKEN_PIPE) 
-    SetEvent (input_available_event);
+  if (output_handle_local)
+    {
+      OBJECT_BASIC_INFORMATION obi;
+      NTSTATUS status;
+      ULONG hdl_cnt = 0;
+
+      status = NtQueryObject (output_handle_local, ObjectBasicInformation,
+	  &obi, sizeof obi, NULL);
+      if (!NT_SUCCESS (status))
+	debug_printf ("NtQueryObject: %y", status);
+      else
+	hdl_cnt = obi.HandleCount;
+      termios_printf("HandleCount: %d", hdl_cnt);
+      if (hdl_cnt == 1)
+	SetEvent (input_available_event);
+      CloseHandle (output_handle_local);
+    }
 
   if (!ForceCloseHandle (from_master))
     termios_printf ("error closing from_master %p, %E", from_master);

[-- Attachment #3: Type: text/plain, Size: 218 bytes --]

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: cygwin-2 process handling

[Ken Brown]

Hi Takashi,

On 5/21/2015 7:53 AM, Takashi Yano wrote:
> I was looking into this problem, and found the PeekNamedPipe()
> call is blocked in fhandler_pty_master::close() when the problem
> occurs.
>
> I had not noticed that,
>
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa365779(v=vs.85).aspx
>
> says:
>> The PeekNamedPipe function can block thread execution the same
>> way any I/O function can when called on a synchronous handle in
>> a multi-threaded application.
>
> I have made a patch attached to resolve this problem. Please have
> a look.

Thanks!  That fixes it for me.  Michael (OP), are you able to build 
cygwin1.dll and test it?

Ken


--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: cygwin-2 process handling

[Corinna Vinschen]

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

Hi Takashi,

On May 21 20:53, Takashi Yano wrote:
> [...]
> I was looking into this problem, and found the PeekNamedPipe()
> call is blocked in fhandler_pty_master::close() when the problem
> occurs.
> 
> I had not noticed that, 
> 
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa365779(v=vs.85).aspx
> 
> says:
> > The PeekNamedPipe function can block thread execution the same
> > way any I/O function can when called on a synchronous handle in
> > a multi-threaded application.
> 
> I have made a patch attached to resolve this problem. Please have
> a look.
> 
> With this patch, NtQueryObject() is used instead of PeekNamedPipe()
> to detect closure of the last master handle.
> 
> Please refer to the following discussion for more details.
> http://cygwin.com/ml/cygwin/2015-03/msg00221.html
> 
> 
> ChageLog is as follows.
> 
> 2015-05-21  Takashi Yano  <...>
> 
> 	* fhandler_tty.cc (fhandler_pty_master::close): Use NtQueryObject()
> 	instead of PeekNamedPipe() to detect closing the last master handle.
> [...]
> +  if (output_handle_local)
> +    {
> +      OBJECT_BASIC_INFORMATION obi;
> +      NTSTATUS status;
> +      ULONG hdl_cnt = 0;
> +
> +      status = NtQueryObject (output_handle_local, ObjectBasicInformation,
> +	  &obi, sizeof obi, NULL);
> +      if (!NT_SUCCESS (status))
> +	debug_printf ("NtQueryObject: %y", status);
> +      else
> +	hdl_cnt = obi.HandleCount;
> +      termios_printf("HandleCount: %d", hdl_cnt);
> +      if (hdl_cnt == 1)
> +	SetEvent (input_available_event);
> +      CloseHandle (output_handle_local);
> +    }
 
Isn't that racy?  Consider two processes doing that at the same time.
Both calls to NtQueryObject could come up with hdl_cnt == 2 and the
problem persists.

Wouldn't it be safer to call SetEvent(input_available_event) all the
time from here?


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: cygwin-2 process handling

[Corinna Vinschen]

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

On May 27 14:23, Corinna Vinschen wrote:
> Hi Takashi,
> 
> On May 21 20:53, Takashi Yano wrote:
> > [...]
> > +  if (output_handle_local)
> > +    {
> > +      OBJECT_BASIC_INFORMATION obi;
> > +      NTSTATUS status;
> > +      ULONG hdl_cnt = 0;
> > +
> > +      status = NtQueryObject (output_handle_local, ObjectBasicInformation,
> > +	  &obi, sizeof obi, NULL);
> > +      if (!NT_SUCCESS (status))
> > +	debug_printf ("NtQueryObject: %y", status);
> > +      else
> > +	hdl_cnt = obi.HandleCount;
> > +      termios_printf("HandleCount: %d", hdl_cnt);
> > +      if (hdl_cnt == 1)
> > +	SetEvent (input_available_event);
> > +      CloseHandle (output_handle_local);
> > +    }
>  
> Isn't that racy?  Consider two processes doing that at the same time.
> Both calls to NtQueryObject could come up with hdl_cnt == 2 and the
> problem persists.
> 
> Wouldn't it be safer to call SetEvent(input_available_event) all the
> time from here?

We discussed this already in March and only briefly talked about a
change like this requiring changes to fhandler_pty_slave::read.
However, I don't see this.  The read code already takes 0 bytes input
and broken pipe scenarios into account.  Do you see something needing
a change I don't?


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: cygwin-2 process handling

[Corinna Vinschen]

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

On May 27 17:17, Corinna Vinschen wrote:
> On May 27 14:23, Corinna Vinschen wrote:
> > Hi Takashi,
> > 
> > On May 21 20:53, Takashi Yano wrote:
> > > [...]
> > > +  if (output_handle_local)
> > > +    {
> > > +      OBJECT_BASIC_INFORMATION obi;
> > > +      NTSTATUS status;
> > > +      ULONG hdl_cnt = 0;
> > > +
> > > +      status = NtQueryObject (output_handle_local, ObjectBasicInformation,
> > > +	  &obi, sizeof obi, NULL);
> > > +      if (!NT_SUCCESS (status))
> > > +	debug_printf ("NtQueryObject: %y", status);
> > > +      else
> > > +	hdl_cnt = obi.HandleCount;
> > > +      termios_printf("HandleCount: %d", hdl_cnt);
> > > +      if (hdl_cnt == 1)
> > > +	SetEvent (input_available_event);
> > > +      CloseHandle (output_handle_local);
> > > +    }
> >  
> > Isn't that racy?  Consider two processes doing that at the same time.
> > Both calls to NtQueryObject could come up with hdl_cnt == 2 and the
> > problem persists.
> > 
> > Wouldn't it be safer to call SetEvent(input_available_event) all the
> > time from here?
> 
> We discussed this already in March and only briefly talked about a
> change like this requiring changes to fhandler_pty_slave::read.
> However, I don't see this.  The read code already takes 0 bytes input
> and broken pipe scenarios into account.  Do you see something needing
> a change I don't?

Never mind, it doesn't work quite as expected, so changes would be
required.

I created another version of your patch which avoids duplicating the
tested handle and makes the test-and-close-handle operation atomic:

	* fhandler_tty.cc (fhandler_pty_common::close): Don't close output_mutex
	here.  Move into callers.
	(fhandler_pty_master::close): Use NtQueryObject instead of
	PeekNamedPipe to detect closing the last master handle.

diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index e91b3e3..12f6124 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -552,7 +552,10 @@ fhandler_pty_slave::close ()
 	get_output_handle_cyg ());
   if ((unsigned) myself->ctty == FHDEV (DEV_PTYS_MAJOR, get_minor ()))
     fhandler_console::free_console ();	/* assumes that we are the last pty closer */
-  return fhandler_pty_common::close ();
+  fhandler_pty_common::close ();
+  if (!ForceCloseHandle (output_mutex))
+    termios_printf ("CloseHandle (output_mutex<%p>), %E", output_mutex);
+  return 0;
 }
 
 int
@@ -1259,8 +1262,6 @@ fhandler_pty_common::close ()
   termios_printf ("pty%d <%p,%p> closing", get_minor (), get_handle (), get_output_handle ());
   if (!ForceCloseHandle (input_mutex))
     termios_printf ("CloseHandle (input_mutex<%p>), %E", input_mutex);
-  if (!ForceCloseHandle (output_mutex))
-    termios_printf ("CloseHandle (output_mutex<%p>), %E", output_mutex);
   if (!ForceCloseHandle1 (get_handle (), from_pty))
     termios_printf ("CloseHandle (get_handle ()<%p>), %E", get_handle ());
   if (!ForceCloseHandle1 (get_output_handle (), to_pty))
@@ -1281,6 +1282,9 @@ fhandler_pty_master::cleanup ()
 int
 fhandler_pty_master::close ()
 {
+  OBJECT_BASIC_INFORMATION obi;
+  NTSTATUS status;
+
   termios_printf ("closing from_master(%p)/to_master(%p)/to_master_cyg(%p) since we own them(%u)",
 		  from_master, to_master, to_master_cyg, dwProcessId);
   if (cygwin_finished_initializing)
@@ -1309,13 +1313,22 @@ fhandler_pty_master::close ()
 	}
     }
 
-  fhandler_pty_common::close ();
-
   /* Check if the last master handle has been closed.  If so, set
      input_available_event to wake up potentially waiting slaves. */
-  if (!PeekNamedPipe (from_master, NULL, 0, NULL, NULL, NULL)
-      && GetLastError () == ERROR_BROKEN_PIPE) 
-    SetEvent (input_available_event);
+  acquire_output_mutex (INFINITE);
+  status = NtQueryObject (get_output_handle (), ObjectBasicInformation,
+			  &obi, sizeof obi, NULL);
+  fhandler_pty_common::close ();
+  release_output_mutex ();
+  if (!ForceCloseHandle (output_mutex))
+    termios_printf ("CloseHandle (output_mutex<%p>), %E", output_mutex);
+  if (!NT_SUCCESS (status))
+    debug_printf ("NtQueryObject: %y", status);
+  else if (obi.HandleCount == 1)
+    {
+      termios_printf("Closing last master");
+      SetEvent (input_available_event);
+    }
 
   if (!ForceCloseHandle (from_master))
     termios_printf ("error closing from_master %p, %E", from_master);

Does that look ok?  It fixes the reported problem for me.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: cygwin-2 process handling

[Takashi Yano]

On Thu, 28 May 2015 13:47:28 +0200
Corinna Vinschen wrote:

> I created another version of your patch which avoids duplicating the
> tested handle and makes the test-and-close-handle operation atomic:

> Does that look ok?  It fixes the reported problem for me.

Oops! I have just sent another patch.

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

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: cygwin-2 process handling

[Takashi Yano]

Hi Corinna,

On Thu, 28 May 2015 21:09:32 +0900
Takashi Yano wrote:

> On Thu, 28 May 2015 13:47:28 +0200
> Corinna Vinschen wrote:
> 
> > I created another version of your patch which avoids duplicating the
> > tested handle and makes the test-and-close-handle operation atomic:
> 
> > Does that look ok?  It fixes the reported problem for me.
> 
> Oops! I have just sent another patch.

Your patch also works nicely for me.

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

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: cygwin-2 process handling

[Corinna Vinschen]

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

Hi Takashi,

On May 28 21:44, Takashi Yano wrote:
> Hi Corinna,
> 
> On Thu, 28 May 2015 21:09:32 +0900
> Takashi Yano wrote:
> 
> > On Thu, 28 May 2015 13:47:28 +0200
> > Corinna Vinschen wrote:
> > 
> > > I created another version of your patch which avoids duplicating the
> > > tested handle and makes the test-and-close-handle operation atomic:
> > 
> > > Does that look ok?  It fixes the reported problem for me.
> > 
> > Oops! I have just sent another patch.
> 
> Your patch also works nicely for me.

I applied this patch.  I'm not comfortable with removing the SIGHUP
handling from slave::read in favor of just setting errno to EIO.
EIO seems wrong here.  Not being able to access the pipe anymore
should be equivalent to a hangup.  Alternatively the return code
from the Windows function should be checked for ERROR_BROKEN_PIPE,
perhaps, but that introduces some more code,


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: cygwin-2 process handling (about SIGHUP)

[Takashi Yano]

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

Hi Corinna,

On Thu, 28 May 2015 16:16:12 +0200 Corinna Vinschen wrote:

> I applied this patch.  I'm not comfortable with removing the SIGHUP
> handling from slave::read in favor of just setting errno to EIO.
> EIO seems wrong here.  Not being able to access the pipe anymore
> should be equivalent to a hangup.  Alternatively the return code
> from the Windows function should be checked for ERROR_BROKEN_PIPE,
> perhaps, but that introduces some more code,

Regarding SIGHUP, I have a different thought.

I have checked the behaviour of PTY on other systems than cygwin.
However, no system raises SIGHUP on the read()/write() access to
PTY, of which the other side is closed. But on the close() in the
master side, only when the slave side of the PTY is the controlling
terminal, SIGHUP is sent to the slave side.

I have tested on Debian GNU/Linux and FreeBSD as well as Cygwin.

The test case is as attached (pipe_pty_test.tgz). The testing
procedure is as follows.

tar xzvf pipe_pty_test.tgz
make
make test

This test case checks how the PTY behaves when the other side
of the PYT is closed. As a comparison, behaviour of pipe is
also tested.

On Debian, the result is:
Linux debian 3.16.0-4-686-pae #1 SMP Debian 3.16.7-ckt9-3~deb8u1 (2015-04-24) i686 GNU/Linux
=== pipe read ==========
R: 0
read(): Success
=== pipe write =========
signal: 13
W: -1
write(): Broken pipe
=== pty master read ====
MR: -1
read(): Input/output error
=== pty master write ===
MW: 11
=== pty slave read =====
SR: -1
read(): Input/output error
=== pty slave write ====
SW: -1
write(): Input/output error
=== pty master close ===
signal: 18
signal: 1
usleep(): Interrupted system call


On FreeBSD, the result is:
FreeBSD FreeBSD-VM 10.1-RELEASE FreeBSD 10.1-RELEASE #0 r274401: Tue Nov 11 22:51:51 UTC 2014     root@releng1.nyi.freebsd.org:/usr/obj/usr/src/sys/GENERIC  i386
=== pipe read ==========
R: 0
read(): No error: 0
=== pipe write =========
signal: 13
W: -1
write(): Broken pipe
=== pty master read ====
MR: 0
read(): No error: 0
=== pty master write ===
MW: 11
=== pty slave read =====
SR: 0
read(): No error: 0
=== pty slave write ====
SW: -1
write(): Device not configured
=== pty master close ===
signal: 1
usleep(): Interrupted system call


However, on cygwin, the result is:
CYGWIN_NT-6.1-WOW Express5800-S70 2.0.3(0.287/5/3) 2015-06-03 13:55 i686 Cygwin
=== pipe read ==========
R: 0
read(): No error
=== pipe write =========
signal: 13
W: -1
write(): Broken pipe
=== pty master read ====
MR: -1
read(): Input/output error
=== pty master write ===
MW: 11
=== pty slave read =====
signal: 1
SR: 0
read(): No error
=== pty slave write ====
signal: 1
SW: -1
write(): Input/output error
=== pty master close ===
usleep(): No error


On cygwin, extra SIGHUPs (signal: 1) are generated in the pty-
slave-read case and in the pty-slave-write case. On the contrary,
SIGHUP is not generated in the pty-master-close case while it
is generated on other systems. On the Debian, SIGCONT (signal:
18) is also sent in the pty-master-close case.

Although SIGHUP on the master closure was pointed out in the
past: https://www.cygwin.com/ml/cygwin/2011-07/msg00328.html,
it was not adopted at that time.

In the track of Debian/FreeBSD, SIGHUP should not be generated
on the read()/write() access on the slave side, but should be
generated on the close() on the master side, only when the slave
side of the PTY is the controlling terminal.

It seems that this behaviour is specified in the POSIX standard.
http://pubs.opengroup.org/onlinepubs/9699919799/functions/close.html

To make the behaviour match with the test results on other
systems, I have made a patch attached (cygwin.patch.20150604).

With this patch, everything works nicely for me as well.

Furthermore, with this patch, mc (midnight commander) becomes
exiting normally. On cygwin 2.0.3-1, mc has a problem which
leaves a subshell process on the background.

To reproduce this problem, execute
env SHELL=/bin/bash mc
on cygwin 2.0.3-1, and type 'exit'.

You will find bash process remains in background. I have confirmed
this problem can be resolved by this patch.

For the reason above, I think it is better to make the behaviour
match with other systems as well as the POSIX specification.

What do you think?


ChangeLog is as follows.

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

	* fhandler_tty.cc (fhandler_pty_slave::write): Move causing of SIGHUP
	into fhandler_pty_master::close().
	(fhandler_pty_slave::read): Ditto.
	(fhandler_pty_master::close): Ditto.

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

[-- Attachment #2: pipe_pty_test.tgz --]
[-- Type: application/x-gtar, Size: 1226 bytes --]

[-- Attachment #3: cygwin.patch.20150604 --]
[-- Type: application/octet-stream, Size: 1818 bytes --]

diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index 1b5c18b..f22998f 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -622,7 +622,6 @@ fhandler_pty_slave::write (const void *ptr, size_t len)
 	default:
 	  __seterrno_from_win_error (err);
 	}
-      raise (SIGHUP);		/* FIXME: Should this be SIGTTOU? */
       towrite = -1;
     }
   return towrite;
@@ -749,7 +748,12 @@ fhandler_pty_slave::read (void *ptr, size_t& len)
 	  goto out;
 	}
       if (!bytes_available (bytes_in_pipe))
-	raise (SIGHUP);
+	{
+	  ReleaseMutex (input_mutex);
+	  set_errno (EIO);
+	  totalread = -1;
+	  goto out;
+	}
 
       /* On first peek determine no. of bytes to flush. */
       if (!ptr && len == UINT_MAX)
@@ -779,9 +783,10 @@ fhandler_pty_slave::read (void *ptr, size_t& len)
 	  if (!ReadFile (get_handle (), buf, readlen, &n, NULL))
 	    {
 	      termios_printf ("read failed, %E");
-	      raise (SIGHUP);
-	      bytes_in_pipe = 0;
-	      ptr = NULL;
+	      ReleaseMutex (input_mutex);
+	      set_errno (EIO);
+	      totalread = -1;
+	      goto out;
 	    }
 	  else
 	    {
@@ -790,7 +795,12 @@ fhandler_pty_slave::read (void *ptr, size_t& len)
 		 change after successful read. So we have to peek into the pipe
 		 again to see if input is still available */
 	      if (!bytes_available (bytes_in_pipe))
-		raise (SIGHUP);
+		{
+		  ReleaseMutex (input_mutex);
+		  set_errno (EIO);
+		  totalread = -1;
+		  goto out;
+		}
 	      if (n)
 		{
 		  len -= n;
@@ -1269,6 +1279,8 @@ fhandler_pty_master::close ()
   else if (obi.HandleCount == 1)
     {
       termios_printf("Closing last master of pty%d", get_minor ());
+      if (get_ttyp ()->getsid () > 0)
+	kill (get_ttyp ()->getsid (), SIGHUP);
       SetEvent (input_available_event);
     }
 

[-- Attachment #4: Type: text/plain, Size: 218 bytes --]

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: cygwin-2 process handling (about SIGHUP)

[Corinna Vinschen]

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

Hi Takashi,

On Jun  7 09:03, Takashi Yano wrote:
> Hi Corinna,
> 
> On Thu, 28 May 2015 16:16:12 +0200 Corinna Vinschen wrote:
> 
> > I applied this patch.  I'm not comfortable with removing the SIGHUP
> > handling from slave::read in favor of just setting errno to EIO.
> > EIO seems wrong here.  Not being able to access the pipe anymore
> > should be equivalent to a hangup.  Alternatively the return code
> > from the Windows function should be checked for ERROR_BROKEN_PIPE,
> > perhaps, but that introduces some more code,
> 
> Regarding SIGHUP, I have a different thought.
> [...]
> To make the behaviour match with the test results on other
> systems, I have made a patch attached (cygwin.patch.20150604).
> 
> With this patch, everything works nicely for me as well.
> 
> Furthermore, with this patch, mc (midnight commander) becomes
> exiting normally. On cygwin 2.0.3-1, mc has a problem which
> leaves a subshell process on the background.
> 
> To reproduce this problem, execute
> env SHELL=/bin/bash mc
> on cygwin 2.0.3-1, and type 'exit'.
> 
> You will find bash process remains in background. I have confirmed
> this problem can be resolved by this patch.
> 
> For the reason above, I think it is better to make the behaviour
> match with other systems as well as the POSIX specification.
> 
> What do you think?

You convinced me.  I applied your patch.  I'll generate a developer
snapshot later today.  It would be nice if it could get a thorough
testing.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: cygwin-2 process handling (about SIGHUP)

[Marco Atzeri]

On 6/8/2015 2:09 PM, Corinna Vinschen wrote:
> Hi Takashi,
>

> You convinced me.  I applied your patch.  I'll generate a developer
> snapshot later today.  It would be nice if it could get a thorough
> testing.
>
>
> Thanks,
> Corinna

I am testing both 32bit and 64 bit version from yesterday
and the long trail of orphan bash processes I had before seems gone
;-)

I think my machine was specially sensible to some type of race
that left bash processes around during complex builds,
specially during high CPU loads.



Regards
Marco



--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: cygwin-2 process handling (about SIGHUP)

[Corinna Vinschen]

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

On Jun  8 15:50, Marco Atzeri wrote:
> On 6/8/2015 2:09 PM, Corinna Vinschen wrote:
> >Hi Takashi,
> >
> 
> >You convinced me.  I applied your patch.  I'll generate a developer
> >snapshot later today.  It would be nice if it could get a thorough
> >testing.
> >
> >
> >Thanks,
> >Corinna
> 
> I am testing both 32bit and 64 bit version from yesterday
> and the long trail of orphan bash processes I had before seems gone
> ;-)

Good news, thank you!


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: cygwin-2 process handling

[Takashi Yano]

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

Hi Corinna,

On Wed, 27 May 2015 14:23:12 +0200
Corinna Vinschen wrote:

> Isn't that racy?  Consider two processes doing that at the same time.
> Both calls to NtQueryObject could come up with hdl_cnt == 2 and the
> problem persists.

Yes, indeed. I didn't consider enough.

> Wouldn't it be safer to call SetEvent(input_available_event) all the
> time from here?

On Wed, 27 May 2015 17:17:34 +0200
Corinna Vinschen <corinna-cygwin@cygwin.com> wrote:

> We discussed this already in March and only briefly talked about a
> change like this requiring changes to fhandler_pty_slave::read.
> However, I don't see this.  The read code already takes 0 bytes input
> and broken pipe scenarios into account.  Do you see something needing
> a change I don't?

Just calling SetEvent(input_available_event) at all time in
fhandler_pty_master::close() causes immediate closure of the
shell on pty at the start.

Some modifications in fhandler_pty_slave::read() seems necessary.

I have made a patch attached, which modifies fhandler_pty_slave::
read() as well. I have not confirmed enough yet, but it seems to
work for me. Please have a look.

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

[-- Attachment #2: cygwin.patch.20150528 --]
[-- Type: application/octet-stream, Size: 2721 bytes --]

diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index e91b3e3..b667278 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -746,7 +746,18 @@ fhandler_pty_slave::read (void *ptr, size_t& len)
 	  goto out;
 	}
       if (!bytes_available (bytes_in_pipe))
-	raise (SIGHUP);
+	{
+	  set_errno (EIO);
+	  totalread = -1;
+	  goto out;
+	}
+
+      if (bytes_in_pipe == 0 && get_ttyp ()->read_retval > 0)
+	{
+	  ResetEvent (input_available_event);
+	  ReleaseMutex (input_mutex);
+	  continue;
+	}
 
       /* On first peek determine no. of bytes to flush. */
       if (!ptr && len == UINT_MAX)
@@ -776,9 +787,9 @@ fhandler_pty_slave::read (void *ptr, size_t& len)
 	  if (!ReadFile (get_handle (), buf, readlen, &n, NULL))
 	    {
 	      termios_printf ("read failed, %E");
-	      raise (SIGHUP);
-	      bytes_in_pipe = 0;
-	      ptr = NULL;
+	      set_errno (EIO);
+	      totalread = -1;
+	      goto out;
 	    }
 	  else
 	    {
@@ -787,7 +798,11 @@ fhandler_pty_slave::read (void *ptr, size_t& len)
 		 change after successful read. So we have to peek into the pipe
 		 again to see if input is still available */
 	      if (!bytes_available (bytes_in_pipe))
-		raise (SIGHUP);
+		{
+		  set_errno (EIO);
+		  totalread = -1;
+		  goto out;
+		}
 	      if (n)
 		{
 		  len -= n;
@@ -1309,14 +1324,6 @@ fhandler_pty_master::close ()
 	}
     }
 
-  fhandler_pty_common::close ();
-
-  /* Check if the last master handle has been closed.  If so, set
-     input_available_event to wake up potentially waiting slaves. */
-  if (!PeekNamedPipe (from_master, NULL, 0, NULL, NULL, NULL)
-      && GetLastError () == ERROR_BROKEN_PIPE) 
-    SetEvent (input_available_event);
-
   if (!ForceCloseHandle (from_master))
     termios_printf ("error closing from_master %p, %E", from_master);
   if (!ForceCloseHandle (to_master))
@@ -1331,8 +1338,15 @@ fhandler_pty_master::close ()
   ForceCloseHandle (echo_w);
   echo_r = echo_w = NULL;
 
+  fhandler_pty_common::close ();
+
   if (have_execed || get_ttyp ()->master_pid != myself->pid)
     termios_printf ("not clearing: %d, master_pid %d", have_execed, get_ttyp ()->master_pid);
+
+  WaitForSingleObject (input_mutex, INFINITE);
+  SetEvent (input_available_event);
+  ReleaseMutex (input_mutex);
+
   if (!ForceCloseHandle (input_available_event))
     termios_printf ("CloseHandle (input_available_event<%p>), %E", input_available_event);
 
diff --git a/winsup/cygwin/tty.cc b/winsup/cygwin/tty.cc
index 3f1077d..f894449 100644
--- a/winsup/cygwin/tty.cc
+++ b/winsup/cygwin/tty.cc
@@ -238,6 +238,7 @@ tty::init ()
   master_pid = 0;
   is_console = false;
   column = 0;
+  read_retval = 1;
 }
 
 HANDLE

[-- Attachment #3: Type: text/plain, Size: 218 bytes --]

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: cygwin-2 process handling

[Michael Mauger]

Ken Brown said:

> Takashi Yano wrote:
>>

>> I have made a patch attached to resolve this problem. Please have
>> a look.
>>
> 
> Thanks! That fixes it for me. Michael (OP), are you able to build cygwin1.dll and test it?
> 
> 
> Ken


Unfortunately, I don't have the ability to build a new cygwin.dll on my work laptop.  I'll 

have to rely upon your testing, but since you were able to replicate my symptoms and the
problem can be traced to a specific change and a documented behavior, I'm feeling pretty
confident in the patch.

Thanks for your effort on this.

MICHAEL MAUGER // FSF Member // GNU Emacs sql-mode maintainer // GNU Linux, GNU Emacs, OwnCloud

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple