This is the mail archive of the cygwin-patches mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: Patch to optionally disable overlapped pipes


> -----Original Message-----
> From: Christopher Faylor
> Sent: Wednesday, December 25, 2013 04:13
>
> On Tue, Dec 24, 2013 at 11:01:21PM -0000, James Johnston wrote:
> >Hi,
> >
> >As I have recently mentioned on the main Cygwin mailing list, Cygwin by
> >default creates FILE_FLAG_OVERLAPPED named pipes for the standard file
> >handles (stdin/stdout/stderr).  These overlapped pipes require all
> >programs using ReadFile/WriteFile to use overlapped I/O when using the
> pipes.
> 
> Thanks for the patch but Cygwin has been using overlapped I/O with pipes
> for many years.  They are a requirement for proper operation with signals.
> We try to be very sparing when adding new options and we're not going to
> add an option to make things work less reliably.

Alright, so what is the solution to my problems (outlined on the main cygwin
list) if not this patch?  I have been using it for a few weeks with no ill
effects.  It fixed all the problems I was having.  Signals work fine, as far
as I can tell.  I think this is going to be something that will benefit
other users.  Can I modify the patch to make it acceptable?  An analysis of
why I think the patch is harmless is below...

The function I modified is fhandler_pipe::create(fhandler_pipe**, unsigned,
int).  This function is a thin wrapper around a more specific
fhandler_pipe::create(LPSECURITY_ATTRIBUTES, PHANDLE, PHANDLE, DWORD, const
char*, DWORD open_mode) with default values for some of the parameters for
that more specific function, and it passes FILE_FLAG_OVERLAPPED by default.
My change involved optionally removing FILE_FLAG_OVERLAPPED from the
default.

Critically, my change does NOT affect any code that uses the
fhandler_pipe::create overload that takes 6 parameters.  If anyone still
wants an overlapped pipe, they can still pass FILE_FLAG_OVERLAPPED to the
create function that takes 6 parameters.  And as it turns out, Cygwin
already does this.  I searched, and checked each place fhandler_pipe::create
is called:

JamesJ at JamesJ-PC /d/AppData/Local/Temp/cygwin-snapshot-20131219-1
$ grep -r "fhandler_pipe::create" .
<snip changelogs and comments>

./winsup/cygwin/fhandler_fifo.cc:  fhandler_pipe::create (sa_buf, (r), (w),
0, fnpipe (), open_mode)
./winsup/cygwin/fhandler_tty.cc:  res = fhandler_pipe::create (&sec_none,
&get_io_handle (), &to_master,
./winsup/cygwin/miscfuncs.cc:  int ret = fhandler_pipe::create (sa, hr, hw,
0, NULL,
./winsup/cygwin/sigproc.cc:  DWORD err = fhandler_pipe::create (sa,
&my_readsig, &my_sendsig,
./winsup/cygwin/tty.cc:  return fhandler_pipe::create (&sec_none, &r, &w,
^--- all the above use 6 parameter overload: they may or may not specify
overlapped pipe today, and my change does NOT affect this code in ANY way.

./winsup/cygwin/pipe.cc:  int res = fhandler_pipe::create (fhs, psize,
mode);
^--- the pipe_worker function uses 3 parameter overload, so it will be
affected by the new pipe_nooverlap flag and may not get an overlapped pipe.
pipe_worker is a static function and is used by the POSIX _pipe, pipe, and
pipe2 functions.

>From the above, we can see that the ONLY code that is affected by my change
is the pipe_worker function, which is used by the POSIX functions for
creating a pipe.  And from further grepping, I couldn't find any places
where Cygwin was using the POSIX pipe functions for purposes other than
implementing other POSIX functions and in libc.

Some counterpoints:

 * From the above search, signal handling code in places like sigproc.cc and
miscfuncs.cc are clearly unaffected, because they use the 6 parameter
overload, which I didn't modify.  If you pass FILE_FLAG_OVERLAPPED to that
function, it will still work and give you an overlapped pipe.

 * Just to test your hypothesis that I broke signals, I copied a few sample
programs off the Internet that use simple signal handling (e.g. custom
Ctrl+C handler, handler for user signal sent from kill).  Everything worked
as expected, nothing looks broken.  If signal handling is broken, I have yet
to find out how!

 * From a practical standpoint, I've been using this for a few weeks now
since I sent the original message.  I haven't had any problems.  And the
.NET programs I was calling work fine now, whereas they did not previously.
It has only benefitted me, with no problems.  I suspect it would benefit
other users as well.

 * Suppose someone does create a non-overlapped pipe, and then tries to do
overlapped I/O on it.  The Win32 API documentation says this should work,
just that the operation will complete synchronously instead of
asynchronously.  Note that this is a situation that has to be considered
anyway when doing asynchronous I/O in Win32: ReadFile/WriteFile says that
the operation may or may not complete synchronously on an asynchronous file
handle.  It is *not* undefined behavior to do overlapped I/O on a
non-overlapped file handle - it is 100% supported - it will just be
synchronous.  In fact, there is an MSKB article about it:
http://support.microsoft.com/kb/156932/en-us

 * I am not a POSIX expert so I don't know every every single API call that
might possibly want to use asynchronous overlapped I/O on a Win32 file
handle, such as the ones returned by pipe/pipe2.  But the obvious would be
POSIX asynchronous I/O.  But Cygwin doesn't even implement these API calls;
all the aio functions in aio.c just return an error code.  Obviously, they
are not widely used in the wild.  If Cygwin did implement POSIX asynchronous
I/O in the future, it would seem that overlapped pipes would then be
required, and the pipe_nooverlap option might be useful for these rare
situations.

 * Why allow the pipe_byte option but not the pipe_nooverlap option?  Both
are required to properly support calling non-Cygwin programs.  If it is
really an issue to add a new CYGWIN flag, I suggest one of the following:
        * Combine the two flags into one, to be used by users who invoke
non-Cygwin programs.
        * Always assume pipe_nooverlap is set, and remove the flag - since
it appears that there won't be any breaking changes anyway.  It can be added
later if POSIX async I/O is implemented.

Best regards,

James Johnston


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]