This is the mail archive of the
cygwin-developers
mailing list for the Cygwin project.
Re: [RFC] Fix expect leaky pipes
- From: Christopher Faylor <me at cgf dot cx>
- To: cygwin-developers at cygwin dot com
- Date: Sun, 28 May 2006 20:06:08 -0400
- Subject: Re: [RFC] Fix expect leaky pipes
- References: <01b601c682aa$75a1b160$a501a8c0@CAM.ARTIMI.COM>
- Reply-to: cygwin-developers at cygwin dot com
On Mon, May 29, 2006 at 12:00:05AM +0100, Dave Korn wrote:
>
> Hi everyone,
>
>
> I've been looking at the problem with expect leaking pipe handles and
>running dog-slow as its handle table fills up (or on my old w2k box, causing
>a complete kernel lock-up), and I *think* I've got it. After adding a whole
>load of termios_printfs to track everything that was going on, it seems like
>there's just an ordering problem in the shutdown of the final master. In
>fhandler_pty_master::close we check whether the master is still alive, and
>if so, only call the fhandler_tty_common::close routine, but if the master
>is no longer alive (and I /think/ that means the same as "if we're closing
>the final handle to it"), we close the from/to master pipes as well.
>
> Now, in expect what happens is that when the child processes have all
>exited and closed their last open handles to the tty, the expect master
>tries to close its fd to the pty master. On entry the master is still
>alive (signified by the respective event existing) but when we call the
>common::close handler it closes the last open handle to the 'master alive'
>event which then goes away. So we've closed the last fd to it but haven't
>closed the from/to master pipes, and that's how they leak. I've fixed the
>problem by re-checking after calling out to the common::close and seeing if
>the master is *now* dead, and closing the master pipes at that time.
>
> Anyway, this isn't quite right, but I need to ask some advice: initially,
>we had (modulo #if-0 and whitespace)
>
>
>int
>fhandler_pty_master::close ()
>{
> if (get_ttyp ()->master_alive ())
> fhandler_tty_common::close ();
> else
> {
> termios_printf ("freeing tty%d (%d)", get_unit (), get_ttyp ()->ntty);
> if (get_ttyp ()->from_master)
> CloseHandle (get_ttyp ()->from_master);
> if (get_ttyp ()->to_master)
> CloseHandle (get_ttyp ()->to_master);
> fhandler_tty_common::close ();
> if (!hExeced)
> get_ttyp ()->init ();
> }
> return 0;
>}
>
>and I've changed that to
>
>int
>fhandler_pty_master::close ()
>{
> if (get_ttyp ()->master_alive ())
> fhandler_tty_common::close ();
> /* That could have closed the last master, so re-check. */
> if (!get_ttyp ()->master_alive ())
> {
> termios_printf ("freeing tty%d (%d)", get_unit (), get_ttyp ()->ntty);
> if (get_ttyp ()->from_master)
> CloseHandle (get_ttyp ()->from_master);
> if (get_ttyp ()->to_master)
> CloseHandle (get_ttyp ()->to_master);
> fhandler_tty_common::close ();
> if (!hExeced)
> get_ttyp ()->init ();
> }
> return 0;
>}
I think it should be ok to just do:
fhandler_tty_common::close ();
if (!get_ttyp ()->master_alive ())
{
termios_printf ("freeing tty%d (%d)", get_unit (), get_ttyp ()->ntty);
if (get_ttyp ()->from_master)
CloseHandle (get_ttyp ()->from_master);
if (get_ttyp ()->to_master)
CloseHandle (get_ttyp ()->to_master);
...
cgf