Rewriting the FIFO code

Rewriting the FIFO code

[Ken Brown]

This is a followup to http://www.cygwin.org/ml/cygwin/2018-12/msg00104.html.

On 12/11/2018 2:40 PM, Corinna Vinschen wrote
 > Perhaps the unfinished, new AF_UNIX
 > sockets code can be reused or partially duplicated to implement FIFOs.

Here's my first idea.  Associated to each FIFO is an abstract listener socket, 
bound to the FIFO path name preceded by NUL.  This is created as soon as there's 
an attempt to open the FIFO.  It acts as a communication hub for all reads from 
or writes to the FIFO.  Each call to fhandler_fifo::open() creates a client 
socket that connects to the hub.  The client is designated as a reader, writer, 
or duplexer, depending on how it was opened.

The hub is created by a process that then monitors all the clients (via 
select()) until one is ready to read or write, as appropriate.  The process runs 
as long as there are are clients connected to the hub.

There are obviously many details to be worked out, but I hope the general idea 
is clear.  Does this seem like a reasonable approach?  Or is it unnecessarily 
complicated?

Ken

Re: Rewriting the FIFO code

[Ken Brown]

On 12/13/2018 6:52 PM, Ken Brown wrote:
> This is a followup to http://www.cygwin.org/ml/cygwin/2018-12/msg00104.html.
> 
> On 12/11/2018 2:40 PM, Corinna Vinschen wrote
>   > Perhaps the unfinished, new AF_UNIX
>   > sockets code can be reused or partially duplicated to implement FIFOs.
> 
> Here's my first idea.  Associated to each FIFO is an abstract listener socket,
> bound to the FIFO path name preceded by NUL.  This is created as soon as there's
> an attempt to open the FIFO.  It acts as a communication hub for all reads from
> or writes to the FIFO.  Each call to fhandler_fifo::open() creates a client
> socket that connects to the hub.  The client is designated as a reader, writer,
> or duplexer, depending on how it was opened.
> 
> The hub is created by a process that then monitors all the clients (via
> select()) until one is ready to read or write, as appropriate.  The process runs
> as long as there are are clients connected to the hub.
> 
> There are obviously many details to be worked out, but I hope the general idea
> is clear.  Does this seem like a reasonable approach?  Or is it unnecessarily
> complicated?

Never mind.  I think I see a simpler way to achieve the same effect.  I'll write 
again when/if I've sorted it out.

Ken

Re: Rewriting the FIFO code

[Ken Brown]

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

On 12/14/2018 8:43 AM, Ken Brown wrote:
> I'll write
> again when/if I've sorted it out.

Hi Corinna,

Here's a new start.  For now, at least, I'm only trying to accommodate one 
reader and several writers.  Maybe later I'll worry about more than one reader.

The attached patch indicates the approach I have in mind.  There's much more to 
do, and I haven't thought about the duplex case yet.  And there isn't enough 
written for me to test it yet, except to make sure it compiles.  But I just want 
to see if you think the approach is reasonable before I continue.

Ken

[-- Attachment #2: 0001-Allow-several-writers-to-open-a-FIFO.patch --]
[-- Type: text/plain, Size: 8648 bytes --]

From 39b525b40af455e1ac1767359b7ff81d9f263a7c Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Mon, 24 Dec 2018 10:36:57 -0500
Subject: [PATCH] Allow several writers to open a FIFO

This is work in progress.

 - A FIFO reader opens a Windows named pipe that allows unlimited instances.
 - Each time the FIFO is opened for writing, a new instance of the pipe
   is created for the writer to connect to.
 - This is accomplished by having the reader start a thread that
   listens for connection attempts.
 - The reader maintains a list of connected writers.
 - When the reader wants to read, it polls the writers to see if
   there's any data to be read.
---
 winsup/cygwin/fhandler.h       |  10 ++-
 winsup/cygwin/fhandler_fifo.cc | 143 ++++++++++++++++++++-------------
 winsup/cygwin/pipe.cc          |   6 +-
 3 files changed, 99 insertions(+), 60 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 9e63867ab..042590883 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1209,7 +1209,8 @@ public:
   int init (HANDLE, DWORD, mode_t, int64_t);
   static int create (fhandler_pipe *[2], unsigned, int);
   static DWORD create (LPSECURITY_ATTRIBUTES, HANDLE *, HANDLE *, DWORD,
-		       const char *, DWORD, int64_t *unique_id = NULL);
+		       const char *, DWORD, int64_t *unique_id = NULL,
+		       DWORD max_instances = 1);
   fhandler_pipe (void *) {}
 
   void copyto (fhandler_base *x)
@@ -1229,12 +1230,17 @@ public:
   }
 };
 
+#define MAX_CLIENTS PIPE_UNLIMITED_INSTANCES
+
 class fhandler_fifo: public fhandler_base_overlapped
 {
   HANDLE read_ready;
   HANDLE write_ready;
+  fhandler_base_overlapped *client[MAX_CLIENTS];
+  int nclients;
   bool __reg2 wait (HANDLE);
   char __reg2 *fifo_name (char *, const char *);
+  bool wait_client ();
 public:
   fhandler_fifo ();
   int open (int, mode_t);
@@ -1250,6 +1256,8 @@ public:
   select_record *select_read (select_stuff *);
   select_record *select_write (select_stuff *);
   select_record *select_except (select_stuff *);
+  int add_client (fhandler_base_overlapped *);
+  void del_client (int);
 
   fhandler_fifo (void *) {}
 
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 5733ec778..13d0f80c3 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -23,7 +23,7 @@
 
 fhandler_fifo::fhandler_fifo ():
   fhandler_base_overlapped (),
-  read_ready (NULL), write_ready (NULL)
+  read_ready (NULL), write_ready (NULL), nclients (0)
 {
   max_atomic_write = DEFAULT_PIPEBUFSIZE;
   need_fork_fixup (true);
@@ -32,7 +32,8 @@ fhandler_fifo::fhandler_fifo ():
 #define fnevent(w) fifo_name (npbuf, w "-event")
 #define fnpipe() fifo_name (npbuf, "fifo")
 #define create_pipe(r, w) \
-  fhandler_pipe::create (sa_buf, (r), (w), 0, fnpipe (), open_mode)
+  fhandler_pipe::create (sa_buf, (r), (w), 0, fnpipe (), open_mode, NULL, \
+			 PIPE_UNLIMITED_INSTANCES)
 
 char *
 fhandler_fifo::fifo_name (char *buf, const char *what)
@@ -153,36 +154,25 @@ fhandler_fifo::open (int flags, mode_t)
       res = error_errno_set;
       goto out;
     }
+  else if (!wait_client ())
+    {
+      res = error_errno_set;
+      goto out;
+    }
 
-  /* If we're writing, it's a little tricky since it is possible that
-     we're attempting to open the other end of a pipe which is already
-     connected.  In that case, we detect ERROR_PIPE_BUSY, reset the
-     read_ready event and wait for the reader to allow us to connect
-     by signalling read_ready.
-
-     Once the pipe has been set up, we signal write_ready.  */
   if (writer)
     {
-      int err;
-      while (1)
-	if (!wait (read_ready))
-	  {
-	    res = error_errno_set;
-	    goto out;
-	  }
-	else if ((err = create_pipe (NULL, &get_io_handle ())) == 0)
-	  break;
-	else if (err == ERROR_PIPE_BUSY)
-	  {
-	    debug_only_printf ("pipe busy");
-	    ResetEvent (read_ready);
-	  }
-	else
-	  {
-	    debug_printf ("create of writer failed");
-	    res = error_set_errno;
-	    goto out;
-	  }
+      if (!wait (read_ready))
+	{
+	  res = error_errno_set;
+	  goto out;
+	}
+      else if (!create_pipe (NULL, &get_io_handle ()))
+	{
+	  debug_printf ("create of writer failed");
+	  res = error_set_errno;
+	  goto out;
+	}
       if (!arm (write_ready))
 	{
 	  res = error_set_errno;
@@ -221,6 +211,45 @@ out:
   return res == success;
 }
 
+int
+fhandler_fifo::add_client (fhandler_base_overlapped *fh)
+{
+  if (nclients == MAX_CLIENTS)
+    {
+      set_errno (EMFILE);
+      return -1;
+    }
+
+  client[nclients++] = fh;
+  return 0;
+}
+
+void
+fhandler_fifo::del_client (int i)
+{
+  client[i]->close ();		/* Do we need to call DisconnectNamedPipe? */
+  delete client[i];		/* Is that enough cleanup? */
+  nclients--;
+  /* Fill in the hole. */
+  if (i < nclients)
+    memmove (client + i, client + i + 1,
+	     (nclients - i) * sizeof (client[i]));
+}
+
+bool
+fhandler_fifo::wait_client ()
+{
+  /* Start a thread that loops forever and does the following:
+      - waits for a client to connect
+      - creates a new instance of the pipe
+      - sets the io_handle of the reader to the new instance
+      - creates a new fhandler_base_overlapped object whose io_handle
+        is the previous handle of the reader
+      - adds a pointer to that object to the client array.
+      - signals read_ready */
+  return true;
+}
+
 off_t
 fhandler_fifo::lseek (off_t offset, int whence)
 {
@@ -282,40 +311,42 @@ fhandler_fifo::wait (HANDLE h)
 void __reg3
 fhandler_fifo::raw_read (void *in_ptr, size_t& len)
 {
-  size_t orig_len = len;
-  for (int i = 0; i < 2; i++)
+  bool keep_looping = true;
+  do
     {
-      fhandler_base_overlapped::raw_read (in_ptr, len);
-      if (len || i || WaitForSingleObject (read_ready, 0) != WAIT_OBJECT_0)
-	break;
-      /* If we got here, then fhandler_base_overlapped::raw_read returned 0,
-	 indicating "EOF" and something has set read_ready to zero.  That means
-	 we should have a client waiting to connect.
-	 FIXME: If the client CTRL-C's the open during this time then this
-	 could hang indefinitely.  Maybe implement a timeout?  */
-      if (!DisconnectNamedPipe (get_io_handle ()))
+      if (nclients == 0)	/* Return EOF. */
 	{
-	  debug_printf ("DisconnectNamedPipe failed, %E");
-	  goto errno_out;
+	  len = 0;
+	  return;
 	}
-      else if (!ConnectNamedPipe (get_io_handle (), get_overlapped ())
-	       && GetLastError () != ERROR_IO_PENDING)
+      int i = 0;
+      while (i < nclients)
 	{
-	  debug_printf ("ConnectNamedPipe failed, %E");
-	  goto errno_out;
+	  size_t orig_len = len;
+	  bool was_nonblocking = client[i]->is_nonblocking ();
+	  client[i]->set_nonblocking (true);
+	  client[i]->raw_read (in_ptr, len);
+	  client[i]->set_nonblocking (was_nonblocking);
+	  if (len > 0)		/* Success. */
+	    {
+	      keep_looping = false;
+	      break;
+	    }
+	  else if (len < 0 && errno != EAGAIN) /* Unexpected error. */
+	    return;
+	  else if (len == 0)	/* Client no longer connected. */
+	    del_client (i);
+	  else			/* No data available to read. */
+	    i++;
+	  len = orig_len;
 	}
-      else if (!arm (read_ready))
-	goto errno_out;
-      else if (!wait (get_overlapped_buffer ()->hEvent))
-	goto errout;	/* If wait() fails, errno is set so no need to set it */
-      len = orig_len;	/* Reset since raw_read above set it to zero. */
+      if (is_nonblocking ())
+	keep_looping = false;
+      else
+	yield ();
     }
+  while (keep_looping);
   return;
-
-errno_out:
-  __seterrno ();
-errout:
-  len = -1;
 }
 
 int __reg2
diff --git a/winsup/cygwin/pipe.cc b/winsup/cygwin/pipe.cc
index f1eace6a6..94de5f2d0 100644
--- a/winsup/cygwin/pipe.cc
+++ b/winsup/cygwin/pipe.cc
@@ -209,7 +209,7 @@ fhandler_pipe::dup (fhandler_base *child, int flags)
 DWORD
 fhandler_pipe::create (LPSECURITY_ATTRIBUTES sa_ptr, PHANDLE r, PHANDLE w,
 		       DWORD psize, const char *name, DWORD open_mode,
-		       int64_t *unique_id)
+		       int64_t *unique_id, DWORD max_instances)
 {
   /* Default to error. */
   if (r)
@@ -274,8 +274,8 @@ fhandler_pipe::create (LPSECURITY_ATTRIBUTES sa_ptr, PHANDLE r, PHANDLE w,
 	 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, open_mode, pipe_mode, 1, psize,
-			   psize, NMPWAIT_USE_DEFAULT_WAIT, sa_ptr);
+      *r = CreateNamedPipe (pipename, open_mode, pipe_mode, max_instances,
+			    psize, psize, NMPWAIT_USE_DEFAULT_WAIT, sa_ptr);
 
       if (*r != INVALID_HANDLE_VALUE)
 	{
-- 
2.17.0

Re: Rewriting the FIFO code

[Corinna Vinschen]

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

On Dec 25 21:14, Ken Brown wrote:
> On 12/14/2018 8:43 AM, Ken Brown wrote:
> > I'll write
> > again when/if I've sorted it out.
> 
> Hi Corinna,
> 
> Here's a new start.  For now, at least, I'm only trying to accommodate one 
> reader and several writers.  Maybe later I'll worry about more than one reader.
> 
> The attached patch indicates the approach I have in mind.  There's much more to 
> do, and I haven't thought about the duplex case yet.  And there isn't enough 
> written for me to test it yet, except to make sure it compiles.  But I just want 
> to see if you think the approach is reasonable before I continue.

Please go ahead.  I'm really excited that you're working on this.  FIFOs
are badly in need of a better approch anyway(*).  Turn the code upside
down, for all it's worth!


Happy Holidays,
Corinna

(*) Like so much other old code in Cygwin...


> 
> Ken

> From 39b525b40af455e1ac1767359b7ff81d9f263a7c Mon Sep 17 00:00:00 2001
> From: Ken Brown <kbrown@cornell.edu>
> Date: Mon, 24 Dec 2018 10:36:57 -0500
> Subject: [PATCH] Allow several writers to open a FIFO
> 
> This is work in progress.
> 
>  - A FIFO reader opens a Windows named pipe that allows unlimited instances.
>  - Each time the FIFO is opened for writing, a new instance of the pipe
>    is created for the writer to connect to.
>  - This is accomplished by having the reader start a thread that
>    listens for connection attempts.
>  - The reader maintains a list of connected writers.
>  - When the reader wants to read, it polls the writers to see if
>    there's any data to be read.
> ---
>  winsup/cygwin/fhandler.h       |  10 ++-
>  winsup/cygwin/fhandler_fifo.cc | 143 ++++++++++++++++++++-------------
>  winsup/cygwin/pipe.cc          |   6 +-
>  3 files changed, 99 insertions(+), 60 deletions(-)
> 
> diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
> index 9e63867ab..042590883 100644
> --- a/winsup/cygwin/fhandler.h
> +++ b/winsup/cygwin/fhandler.h
> @@ -1209,7 +1209,8 @@ public:
>    int init (HANDLE, DWORD, mode_t, int64_t);
>    static int create (fhandler_pipe *[2], unsigned, int);
>    static DWORD create (LPSECURITY_ATTRIBUTES, HANDLE *, HANDLE *, DWORD,
> -		       const char *, DWORD, int64_t *unique_id = NULL);
> +		       const char *, DWORD, int64_t *unique_id = NULL,
> +		       DWORD max_instances = 1);
>    fhandler_pipe (void *) {}
>  
>    void copyto (fhandler_base *x)
> @@ -1229,12 +1230,17 @@ public:
>    }
>  };
>  
> +#define MAX_CLIENTS PIPE_UNLIMITED_INSTANCES
> +
>  class fhandler_fifo: public fhandler_base_overlapped
>  {
>    HANDLE read_ready;
>    HANDLE write_ready;
> +  fhandler_base_overlapped *client[MAX_CLIENTS];
> +  int nclients;
>    bool __reg2 wait (HANDLE);
>    char __reg2 *fifo_name (char *, const char *);
> +  bool wait_client ();
>  public:
>    fhandler_fifo ();
>    int open (int, mode_t);
> @@ -1250,6 +1256,8 @@ public:
>    select_record *select_read (select_stuff *);
>    select_record *select_write (select_stuff *);
>    select_record *select_except (select_stuff *);
> +  int add_client (fhandler_base_overlapped *);
> +  void del_client (int);
>  
>    fhandler_fifo (void *) {}
>  
> diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
> index 5733ec778..13d0f80c3 100644
> --- a/winsup/cygwin/fhandler_fifo.cc
> +++ b/winsup/cygwin/fhandler_fifo.cc
> @@ -23,7 +23,7 @@
>  
>  fhandler_fifo::fhandler_fifo ():
>    fhandler_base_overlapped (),
> -  read_ready (NULL), write_ready (NULL)
> +  read_ready (NULL), write_ready (NULL), nclients (0)
>  {
>    max_atomic_write = DEFAULT_PIPEBUFSIZE;
>    need_fork_fixup (true);
> @@ -32,7 +32,8 @@ fhandler_fifo::fhandler_fifo ():
>  #define fnevent(w) fifo_name (npbuf, w "-event")
>  #define fnpipe() fifo_name (npbuf, "fifo")
>  #define create_pipe(r, w) \
> -  fhandler_pipe::create (sa_buf, (r), (w), 0, fnpipe (), open_mode)
> +  fhandler_pipe::create (sa_buf, (r), (w), 0, fnpipe (), open_mode, NULL, \
> +			 PIPE_UNLIMITED_INSTANCES)
>  
>  char *
>  fhandler_fifo::fifo_name (char *buf, const char *what)
> @@ -153,36 +154,25 @@ fhandler_fifo::open (int flags, mode_t)
>        res = error_errno_set;
>        goto out;
>      }
> +  else if (!wait_client ())
> +    {
> +      res = error_errno_set;
> +      goto out;
> +    }
>  
> -  /* If we're writing, it's a little tricky since it is possible that
> -     we're attempting to open the other end of a pipe which is already
> -     connected.  In that case, we detect ERROR_PIPE_BUSY, reset the
> -     read_ready event and wait for the reader to allow us to connect
> -     by signalling read_ready.
> -
> -     Once the pipe has been set up, we signal write_ready.  */
>    if (writer)
>      {
> -      int err;
> -      while (1)
> -	if (!wait (read_ready))
> -	  {
> -	    res = error_errno_set;
> -	    goto out;
> -	  }
> -	else if ((err = create_pipe (NULL, &get_io_handle ())) == 0)
> -	  break;
> -	else if (err == ERROR_PIPE_BUSY)
> -	  {
> -	    debug_only_printf ("pipe busy");
> -	    ResetEvent (read_ready);
> -	  }
> -	else
> -	  {
> -	    debug_printf ("create of writer failed");
> -	    res = error_set_errno;
> -	    goto out;
> -	  }
> +      if (!wait (read_ready))
> +	{
> +	  res = error_errno_set;
> +	  goto out;
> +	}
> +      else if (!create_pipe (NULL, &get_io_handle ()))
> +	{
> +	  debug_printf ("create of writer failed");
> +	  res = error_set_errno;
> +	  goto out;
> +	}
>        if (!arm (write_ready))
>  	{
>  	  res = error_set_errno;
> @@ -221,6 +211,45 @@ out:
>    return res == success;
>  }
>  
> +int
> +fhandler_fifo::add_client (fhandler_base_overlapped *fh)
> +{
> +  if (nclients == MAX_CLIENTS)
> +    {
> +      set_errno (EMFILE);
> +      return -1;
> +    }
> +
> +  client[nclients++] = fh;
> +  return 0;
> +}
> +
> +void
> +fhandler_fifo::del_client (int i)
> +{
> +  client[i]->close ();		/* Do we need to call DisconnectNamedPipe? */
> +  delete client[i];		/* Is that enough cleanup? */
> +  nclients--;
> +  /* Fill in the hole. */
> +  if (i < nclients)
> +    memmove (client + i, client + i + 1,
> +	     (nclients - i) * sizeof (client[i]));
> +}
> +
> +bool
> +fhandler_fifo::wait_client ()
> +{
> +  /* Start a thread that loops forever and does the following:
> +      - waits for a client to connect
> +      - creates a new instance of the pipe
> +      - sets the io_handle of the reader to the new instance
> +      - creates a new fhandler_base_overlapped object whose io_handle
> +        is the previous handle of the reader
> +      - adds a pointer to that object to the client array.
> +      - signals read_ready */
> +  return true;
> +}
> +
>  off_t
>  fhandler_fifo::lseek (off_t offset, int whence)
>  {
> @@ -282,40 +311,42 @@ fhandler_fifo::wait (HANDLE h)
>  void __reg3
>  fhandler_fifo::raw_read (void *in_ptr, size_t& len)
>  {
> -  size_t orig_len = len;
> -  for (int i = 0; i < 2; i++)
> +  bool keep_looping = true;
> +  do
>      {
> -      fhandler_base_overlapped::raw_read (in_ptr, len);
> -      if (len || i || WaitForSingleObject (read_ready, 0) != WAIT_OBJECT_0)
> -	break;
> -      /* If we got here, then fhandler_base_overlapped::raw_read returned 0,
> -	 indicating "EOF" and something has set read_ready to zero.  That means
> -	 we should have a client waiting to connect.
> -	 FIXME: If the client CTRL-C's the open during this time then this
> -	 could hang indefinitely.  Maybe implement a timeout?  */
> -      if (!DisconnectNamedPipe (get_io_handle ()))
> +      if (nclients == 0)	/* Return EOF. */
>  	{
> -	  debug_printf ("DisconnectNamedPipe failed, %E");
> -	  goto errno_out;
> +	  len = 0;
> +	  return;
>  	}
> -      else if (!ConnectNamedPipe (get_io_handle (), get_overlapped ())
> -	       && GetLastError () != ERROR_IO_PENDING)
> +      int i = 0;
> +      while (i < nclients)
>  	{
> -	  debug_printf ("ConnectNamedPipe failed, %E");
> -	  goto errno_out;
> +	  size_t orig_len = len;
> +	  bool was_nonblocking = client[i]->is_nonblocking ();
> +	  client[i]->set_nonblocking (true);
> +	  client[i]->raw_read (in_ptr, len);
> +	  client[i]->set_nonblocking (was_nonblocking);
> +	  if (len > 0)		/* Success. */
> +	    {
> +	      keep_looping = false;
> +	      break;
> +	    }
> +	  else if (len < 0 && errno != EAGAIN) /* Unexpected error. */
> +	    return;
> +	  else if (len == 0)	/* Client no longer connected. */
> +	    del_client (i);
> +	  else			/* No data available to read. */
> +	    i++;
> +	  len = orig_len;
>  	}
> -      else if (!arm (read_ready))
> -	goto errno_out;
> -      else if (!wait (get_overlapped_buffer ()->hEvent))
> -	goto errout;	/* If wait() fails, errno is set so no need to set it */
> -      len = orig_len;	/* Reset since raw_read above set it to zero. */
> +      if (is_nonblocking ())
> +	keep_looping = false;
> +      else
> +	yield ();
>      }
> +  while (keep_looping);
>    return;
> -
> -errno_out:
> -  __seterrno ();
> -errout:
> -  len = -1;
>  }
>  
>  int __reg2
> diff --git a/winsup/cygwin/pipe.cc b/winsup/cygwin/pipe.cc
> index f1eace6a6..94de5f2d0 100644
> --- a/winsup/cygwin/pipe.cc
> +++ b/winsup/cygwin/pipe.cc
> @@ -209,7 +209,7 @@ fhandler_pipe::dup (fhandler_base *child, int flags)
>  DWORD
>  fhandler_pipe::create (LPSECURITY_ATTRIBUTES sa_ptr, PHANDLE r, PHANDLE w,
>  		       DWORD psize, const char *name, DWORD open_mode,
> -		       int64_t *unique_id)
> +		       int64_t *unique_id, DWORD max_instances)
>  {
>    /* Default to error. */
>    if (r)
> @@ -274,8 +274,8 @@ fhandler_pipe::create (LPSECURITY_ATTRIBUTES sa_ptr, PHANDLE r, PHANDLE w,
>  	 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, open_mode, pipe_mode, 1, psize,
> -			   psize, NMPWAIT_USE_DEFAULT_WAIT, sa_ptr);
> +      *r = CreateNamedPipe (pipename, open_mode, pipe_mode, max_instances,
> +			    psize, psize, NMPWAIT_USE_DEFAULT_WAIT, sa_ptr);
>  
>        if (*r != INVALID_HANDLE_VALUE)
>  	{
> -- 
> 2.17.0
> 


-- 
Corinna Vinschen
Cygwin Maintainer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Rewriting the FIFO code

[Ken Brown]

On 12/26/2018 6:37 AM, Corinna Vinschen wrote:
> On Dec 25 21:14, Ken Brown wrote:
>> On 12/14/2018 8:43 AM, Ken Brown wrote:
>>> I'll write
>>> again when/if I've sorted it out.
>>
>> Hi Corinna,
>>
>> Here's a new start.  For now, at least, I'm only trying to accommodate one
>> reader and several writers.  Maybe later I'll worry about more than one reader.
>>
>> The attached patch indicates the approach I have in mind.  There's much more to
>> do, and I haven't thought about the duplex case yet.  And there isn't enough
>> written for me to test it yet, except to make sure it compiles.  But I just want
>> to see if you think the approach is reasonable before I continue.
> 
> Please go ahead.  I'm really excited that you're working on this.  FIFOs
> are badly in need of a better approch anyway(*).  Turn the code upside
> down, for all it's worth!

Thanks for the encouragement, and Happy Holidays to you too.  One question: In 
the new AF_UNIX socket code you mostly used NT functions, but the existing FIFO 
code uses Win32 functions.  Do you prefer NT functions for new code?

Ken

Re: Rewriting the FIFO code

[Corinna Vinschen]

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

On Dec 26 14:00, Ken Brown wrote:
> On 12/26/2018 6:37 AM, Corinna Vinschen wrote:
> > On Dec 25 21:14, Ken Brown wrote:
> >> On 12/14/2018 8:43 AM, Ken Brown wrote:
> >>> I'll write
> >>> again when/if I've sorted it out.
> >>
> >> Hi Corinna,
> >>
> >> Here's a new start.  For now, at least, I'm only trying to accommodate one
> >> reader and several writers.  Maybe later I'll worry about more than one reader.
> >>
> >> The attached patch indicates the approach I have in mind.  There's much more to
> >> do, and I haven't thought about the duplex case yet.  And there isn't enough
> >> written for me to test it yet, except to make sure it compiles.  But I just want
> >> to see if you think the approach is reasonable before I continue.
> > 
> > Please go ahead.  I'm really excited that you're working on this.  FIFOs
> > are badly in need of a better approch anyway(*).  Turn the code upside
> > down, for all it's worth!
> 
> Thanks for the encouragement, and Happy Holidays to you too.  One question: In 
> the new AF_UNIX socket code you mostly used NT functions, but the existing FIFO 
> code uses Win32 functions.  Do you prefer NT functions for new code?

The NT functions have some advantages over the Win32 functions.
For instance, WaitNamedPipe is not interruptible, while
NtFsControlFile(FSCTL_PIPE_WAIT) can be called asynchronously
and then you can just wait for an event object via cygwait
(see fhandler_socket_unix::wait_pipe_thread).

So, in theory I'd prefer NT functions, but if you feel uncomfortable
and just want to implement away, feel free to go ahead with Win32
functions.


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Rewriting the FIFO code

[Ken Brown]

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

On 12/26/2018 2:03 PM, Corinna Vinschen wrote:
> On Dec 26 14:00, Ken Brown wrote:
>> Thanks for the encouragement, and Happy Holidays to you too.  One question: In
>> the new AF_UNIX socket code you mostly used NT functions, but the existing FIFO
>> code uses Win32 functions.  Do you prefer NT functions for new code?
> 
> The NT functions have some advantages over the Win32 functions.
> For instance, WaitNamedPipe is not interruptible, while
> NtFsControlFile(FSCTL_PIPE_WAIT) can be called asynchronously
> and then you can just wait for an event object via cygwait
> (see fhandler_socket_unix::wait_pipe_thread).
> 
> So, in theory I'd prefer NT functions, but if you feel uncomfortable
> and just want to implement away, feel free to go ahead with Win32
> functions.

I've decided to use Win32 functions for now, not because of discomfort but 
because it allowed me to use more of the existing code.  I can always change 
that later.

I've got enough working now that I think my patches could use some review.  But 
there's still a lot more to do.  I've attached the patches, but I could send 
them to cygwin-patches instead if you prefer.

Ken

[-- Attachment #2: 0000-cover-letter.patch --]
[-- Type: text/plain, Size: 2589 bytes --]

From 70e60d969c4006ddce3a4e16549d0271158ba8f4 Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Tue, 22 Jan 2019 15:09:35 -0500
Subject: [PATCH FIFO, draft 0/4] Allow a FIFO to have multiple writers

Currently a FIFO can have only one writer.  A second attempt to open
the FIFO for writing blocks while fhandler_fifo::open waits for the
read_ready event to be signalled.

This patch series tries to fix the problem by having the reader open
multiple instances of the Windows named pipe underlying the FIFO.
When the FIFO is opened for reading, a 'listen_client' thread is
created that runs until the FIFO is closed.  This thread listens for
clients (writers) to connect to the pipe, and it creates new pipe
instances as needed.

fhandler_fifo::raw_read loops through the connected writers, checking
for input.

Much remains to be implemented (see below), but preliminary testing of
the current draft seems to work as expected.  I've tested it by
running the fifo client and server programs from Chapter 44 of the
book "The Linux Programming Interface: Linux and UNIX System
Programming Handbook" by Michael Kerrisk.  (See
https://cygwin.com/ml/cygwin/2015-03/msg00047.html for simplified
versions of these programs.  These work as on Linux.)

I've also tried the test given in
http://www.cygwin.org/ml/cygwin/2015-12/msg00311.html.  This doesn't
fully succeed, but I think the failure results from some of the
fhandler_fifo methods that I haven't fully implemented yet.  What
happens is that fifo1.sh in Terminal 1 is correctly unblocked when I
run fifo2.sh in Terminal 2, but fifo2.sh gives the following error:

  -bash: read: read error: 0: Bad file descriptor

I hope to fix that soon.

TODO:

 - Make whatever changes are needed in the fhandler_fifo methods that
   I haven't touched yet.  I hope these changes will fix the "Bad file
   descriptor" error mentioned above.

 - Think about whether shared memory is needed in case several file
   descriptors refer to the same FIFO opened for reading.

 - Try to get the code to work for duplexers (FIFOs opened for reading
   and writing).  I haven't thought about this at all yet.

Ken Brown (4):
  Cygwin: fhandler_fifo: allow unlimited pipe_instances
  Cygwin: fhandler_fifo: Allow multiple writers
  Cygwin: adapt fhandler_fifo::close to recent changes
  Cygwin: fhandler_fifo: add a spinlock

 winsup/cygwin/fhandler.h       |  23 +-
 winsup/cygwin/fhandler_fifo.cc | 426 ++++++++++++++++++++++++++-------
 winsup/cygwin/fhandler_pipe.cc |  11 +-
 3 files changed, 369 insertions(+), 91 deletions(-)

-- 
2.17.0


[-- Attachment #3: 0001-Cygwin-fhandler_fifo-allow-unlimited-pipe_instances.patch --]
[-- Type: text/plain, Size: 4253 bytes --]

From a9b2ac60c8691bd75ddb642b965b65519deb2f11 Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Sun, 13 Jan 2019 11:15:50 -0500
Subject: [PATCH FIFO, draft 1/4] Cygwin: fhandler_fifo: allow unlimited
 pipe_instances

Give fhandler_pipe::create new optional parameters 'max_instances'
(= 1 by default) and first_instance (= true by default).  Use
FILE_FLAG_FIRST_PIPE_INSTANCE only if first_instance is true.

Change the create_pipe macro for fhandler_fifo to call
fhandler_pipe::create with max_instances = PIPE_UNLIMITED_INSTANCES.
---
 winsup/cygwin/fhandler.h       |  3 ++-
 winsup/cygwin/fhandler_fifo.cc |  9 +++++----
 winsup/cygwin/fhandler_pipe.cc | 11 +++++++----
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 7e460701c..5ff00c4d7 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1212,7 +1212,8 @@ public:
   int init (HANDLE, DWORD, mode_t, int64_t);
   static int create (fhandler_pipe *[2], unsigned, int);
   static DWORD create (LPSECURITY_ATTRIBUTES, HANDLE *, HANDLE *, DWORD,
-		       const char *, DWORD, int64_t *unique_id = NULL);
+		       const char *, DWORD, int64_t *unique_id = NULL,
+		       DWORD max_instances = 1, bool first_instance = true);
   fhandler_pipe (void *) {}
 
   void copyto (fhandler_base *x)
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 5733ec778..b171719b1 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -31,8 +31,9 @@ fhandler_fifo::fhandler_fifo ():
 
 #define fnevent(w) fifo_name (npbuf, w "-event")
 #define fnpipe() fifo_name (npbuf, "fifo")
-#define create_pipe(r, w) \
-  fhandler_pipe::create (sa_buf, (r), (w), 0, fnpipe (), open_mode)
+#define create_pipe(r, w, f)					    \
+  fhandler_pipe::create (sa_buf, (r), (w), 0, fnpipe (), open_mode, \
+			 NULL, PIPE_UNLIMITED_INSTANCES, (f))
 
 char *
 fhandler_fifo::fifo_name (char *buf, const char *what)
@@ -137,7 +138,7 @@ fhandler_fifo::open (int flags, mode_t)
      FIXME: Probably need to special case O_RDWR case.  */
   if (!reader)
     /* We are not a reader */;
-  else if (create_pipe (&get_io_handle (), NULL))
+  else if (create_pipe (&get_io_handle (), NULL, true))
     {
       debug_printf ("create of reader failed");
       res = error_set_errno;
@@ -170,7 +171,7 @@ fhandler_fifo::open (int flags, mode_t)
 	    res = error_errno_set;
 	    goto out;
 	  }
-	else if ((err = create_pipe (NULL, &get_io_handle ())) == 0)
+	else if ((err = create_pipe (NULL, &get_io_handle (), false)) == 0)
 	  break;
 	else if (err == ERROR_PIPE_BUSY)
 	  {
diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
index eafaa8856..f09035f16 100644
--- a/winsup/cygwin/fhandler_pipe.cc
+++ b/winsup/cygwin/fhandler_pipe.cc
@@ -216,7 +216,8 @@ fhandler_pipe::dup (fhandler_base *child, int flags)
 DWORD
 fhandler_pipe::create (LPSECURITY_ATTRIBUTES sa_ptr, PHANDLE r, PHANDLE w,
 		       DWORD psize, const char *name, DWORD open_mode,
-		       int64_t *unique_id)
+		       int64_t *unique_id, DWORD max_instances,
+		       bool first_instance)
 {
   /* Default to error. */
   if (r)
@@ -246,7 +247,9 @@ fhandler_pipe::create (LPSECURITY_ATTRIBUTES sa_ptr, PHANDLE r, PHANDLE w,
   if (name)
     len += __small_sprintf (pipename + len, "%s", name);
 
-  open_mode |= PIPE_ACCESS_INBOUND | FILE_FLAG_FIRST_PIPE_INSTANCE;
+  open_mode |= PIPE_ACCESS_INBOUND;
+  if (first_instance)
+    open_mode |= FILE_FLAG_FIRST_PIPE_INSTANCE;
 
   /* Retry CreateNamedPipe as long as the pipe name is in use.
      Retrying will probably never be necessary, but we want
@@ -281,8 +284,8 @@ fhandler_pipe::create (LPSECURITY_ATTRIBUTES sa_ptr, PHANDLE r, PHANDLE w,
 	 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, open_mode, pipe_mode, 1, psize,
-			   psize, NMPWAIT_USE_DEFAULT_WAIT, sa_ptr);
+      *r = CreateNamedPipe (pipename, open_mode, pipe_mode, max_instances,
+			    psize, psize, NMPWAIT_USE_DEFAULT_WAIT, sa_ptr);
 
       if (*r != INVALID_HANDLE_VALUE)
 	{
-- 
2.17.0


[-- Attachment #4: 0002-Cygwin-fhandler_fifo-Allow-multiple-writers.patch --]
[-- Type: text/plain, Size: 13444 bytes --]

From 261fe9fe39e3e0587df302960f6052ebc2dd514a Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Sun, 13 Jan 2019 15:11:12 -0500
Subject: [PATCH FIFO, draft 2/4] Cygwin: fhandler_fifo: Allow multiple writers

Introduce a 'pipe_instance' structure that can be used by a reader to
communicate with a writer using an instance of the named pipe.  An
fhandler_fifo opened for reading creates a thread that does the
following:

 - maintains a list of pipe_instances
 - listens for clients trying to connect
 - creates new pipe_instances as needed so that there's always at
   least one available for connecting.

fhandler_fifo::raw_read now loops through the connected pipe_instances
and reads from the first one that has data available.
---
 winsup/cygwin/fhandler.h       |  17 ++
 winsup/cygwin/fhandler_fifo.cc | 387 ++++++++++++++++++++++++++-------
 2 files changed, 320 insertions(+), 84 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 5ff00c4d7..311cf0958 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1233,14 +1233,31 @@ public:
   }
 };
 
+#define MAX_INSTANCES PIPE_UNLIMITED_INSTANCES
+
 class fhandler_fifo: public fhandler_base_overlapped
 {
+  struct pipe_instance
+  {
+    fhandler_base_overlapped *fh;
+    bool connected;
+    HANDLE dummy_evt;		/* Never signaled. */
+  };
   HANDLE read_ready;
   HANDLE write_ready;
+  HANDLE listen_client_thr;
+  HANDLE lct_termination_evt;
+  pipe_instance inst[MAX_INSTANCES];
+  int ninstances, nconnected;
   bool __reg2 wait (HANDLE);
   char __reg2 *fifo_name (char *, const char *);
+  int disconnect_and_reconnect (int);
+  int create_pipe_instance ();
+  bool listen_client ();
+  bool check_listen_client_thread () const;
 public:
   fhandler_fifo ();
+  DWORD listen_client_thread ();
   int open (int, mode_t);
   off_t lseek (off_t offset, int whence);
   int close ();
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index b171719b1..13d87eab5 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -22,8 +22,9 @@
 #include "cygwait.h"
 
 fhandler_fifo::fhandler_fifo ():
-  fhandler_base_overlapped (),
-  read_ready (NULL), write_ready (NULL)
+  fhandler_base_overlapped (), read_ready (NULL), write_ready (NULL),
+  listen_client_thr (NULL), lct_termination_evt (NULL), ninstances (0),
+  nconnected (0)
 {
   max_atomic_write = DEFAULT_PIPEBUFSIZE;
   need_fork_fixup (true);
@@ -74,6 +75,198 @@ fhandler_fifo::arm (HANDLE h)
   return res;
 }
 
+static int
+connect_to_new_client (fhandler_base_overlapped *fh)
+{
+  bool connected;
+  bool res = ConnectNamedPipe (fh->get_handle (), fh->get_overlapped ());
+  if (res)
+    return -1;
+  switch (GetLastError ())
+    {
+    case ERROR_IO_PENDING:
+      connected = false;
+      break;
+    case ERROR_PIPE_CONNECTED:
+      connected = true;
+      ResetEvent (fh->get_overlapped ()->hEvent);
+      break;
+    default:
+      return -1;
+    }
+  return connected;
+}
+
+int
+fhandler_fifo::disconnect_and_reconnect (int i)
+{
+  int connected;
+
+  inst[i].connected = false;
+  nconnected--;
+  if (!DisconnectNamedPipe (inst[i].fh->get_io_handle ()))
+    {
+      debug_printf ("DisconnectNamedPipe failed, %E");
+      goto errout;
+    }
+  else if ((connected = connect_to_new_client (inst[i].fh)) < 0)
+    goto errout;
+  else
+    inst[i].connected = connected;
+  if (connected)
+    nconnected++;
+  return 0;
+errout:
+  __seterrno ();
+  return -1;
+}
+
+int
+fhandler_fifo::create_pipe_instance ()
+{
+  pipe_instance pi;
+  fhandler_base_overlapped *fh;
+  int connected;
+  bool first_instance = (ninstances == 0);
+  DWORD open_mode = FILE_FLAG_OVERLAPPED;
+
+  if (ninstances == MAX_INSTANCES)
+    {
+      set_errno (EMFILE);
+      return -1;
+    }
+  if (!(pi.dummy_evt = CreateEvent (NULL, true, false, NULL)))
+    {
+      __seterrno ();
+      return -1;
+    }
+  if (!(fh = (fhandler_base_overlapped *) build_fh_dev (dev ())))
+    {
+      set_errno (EMFILE);
+      return -1;
+    }
+  pi.fh = fh;
+  char char_sa_buf[1024];
+  LPSECURITY_ATTRIBUTES sa_buf;
+  sa_buf = sec_user_cloexec (get_flags () & O_CLOEXEC,
+			     (PSECURITY_ATTRIBUTES) char_sa_buf,
+			     cygheap->user.sid());
+  char npbuf[MAX_PATH];
+  HANDLE h;
+  if (create_pipe (&h, NULL, first_instance))
+    goto errout;
+  fh->set_io_handle (h);
+  /* To play it safe, make sure this fhandler_fifo has an io_handle. */
+  if (first_instance)
+    set_io_handle (h);
+  fh->set_flags (get_flags ());
+  if (fh->setup_overlapped ())
+    goto errout;
+  if ((connected = connect_to_new_client (fh)) < 0)
+    goto errout;
+  if ((pi.connected = connected))
+    nconnected++;
+  inst[ninstances++] = pi;
+  return 0;
+errout:
+  __seterrno ();
+  delete fh;
+  return -1;
+}
+
+/* Just hop to the listen_client_thread method. */
+DWORD WINAPI
+listen_client_func (LPVOID param)
+{
+  fhandler_fifo *fh = (fhandler_fifo *) param;
+  return fh->listen_client_thread ();
+}
+
+/* Start a thread that listens for client connections.  Whenever a new
+   client connects, it creates a new pipe_instance if necessary.
+   (There may already be an available instance if a client has
+   disconnected.)  */
+bool
+fhandler_fifo::listen_client ()
+{
+  if (!(lct_termination_evt = CreateEvent (NULL, true, false, NULL)))
+    {
+      __seterrno ();
+      return false;
+    }
+
+  listen_client_thr = CreateThread (NULL, PREFERRED_IO_BLKSIZE,
+				    listen_client_func, (PVOID) this, 0, NULL);
+  if (!listen_client_thr)
+    {
+      __seterrno ();
+      HANDLE evt = InterlockedExchangePointer (&lct_termination_evt, NULL);
+      if (evt)
+	CloseHandle (evt);
+      return false;
+    }
+  return true;
+}
+
+DWORD
+fhandler_fifo::listen_client_thread ()
+{
+  while (1)
+    {
+      if (nconnected == ninstances && create_pipe_instance () < 0)
+	return -1;
+
+      /* Wait for a client to connect. */
+      HANDLE w[MAX_INSTANCES + 1];
+      int i;
+      DWORD wait_ret;
+      for (i = 0; i < ninstances; i++)
+	w[i] = inst[i].connected ? inst[i].dummy_evt
+	  : inst[i].fh->get_overlapped ()->hEvent;
+      w[ninstances] = lct_termination_evt;
+      if (!arm (read_ready))
+	goto errout;
+      wait_ret = WaitForMultipleObjects (ninstances + 1, w, false, INFINITE);
+      i = wait_ret - WAIT_OBJECT_0;
+      if (i < 0 || i > ninstances)
+	goto errout;
+      else if (i == ninstances)	/* Reader is closing. */
+	return 0;
+      else
+	{
+	  inst[i].connected = true;
+	  nconnected++;
+	}
+    }
+errout:
+  __seterrno ();
+  ResetEvent (read_ready);
+  return -1;
+}
+
+/* Is the thread still running? */
+bool
+fhandler_fifo::check_listen_client_thread () const
+{
+  bool ret = false;
+  switch (WaitForSingleObject (listen_client_thr, 0))
+    {
+    case WAIT_OBJECT_0:
+      DWORD err;
+      GetExitCodeThread (listen_client_thr, &err);
+      __seterrno_from_win_error (err);
+      break;
+    case WAIT_TIMEOUT:
+      ret = true;
+      break;
+    case WAIT_FAILED:
+    default:
+      __seterrno ();
+      break;
+    }
+  return ret;
+}
+
 int
 fhandler_fifo::open (int flags, mode_t)
 {
@@ -133,73 +326,81 @@ fhandler_fifo::open (int flags, mode_t)
       goto out;
     }
 
-  /* If we're reading, create the pipe, signal that we're ready and wait for
-     a writer.
+  /* If we're reading, start the listen_client thread (which should
+     signal read_ready), and wait for a writer.
+
      FIXME: Probably need to special case O_RDWR case.  */
-  if (!reader)
-    /* We are not a reader */;
-  else if (create_pipe (&get_io_handle (), NULL, true))
-    {
-      debug_printf ("create of reader failed");
-      res = error_set_errno;
-      goto out;
-    }
-  else if (!arm (read_ready))
-    {
-      res = error_set_errno;
-      goto out;
-    }
-  else if (!duplexer && !wait (write_ready))
+  if (reader)
     {
-      res = error_errno_set;
-      goto out;
+      bool thr_ok = listen_client ();
+      if (!thr_ok)
+	{
+	  debug_printf ("create of listen_client thread failed");
+	  res = error_errno_set;
+	  goto out;
+	}
+      /* Wait for the listen_client thread to create the pipe and
+	 signal read_ready.  This should be quick.  */
+      HANDLE w[2] = { listen_client_thr, read_ready };
+      switch (WaitForMultipleObjects (2, w, FALSE, INFINITE))
+	{
+	case WAIT_OBJECT_0:
+	  debug_printf ("listen_client_thread exited unexpectedly");
+	  DWORD err;
+	  GetExitCodeThread (listen_client_thr, &err);
+	  __seterrno_from_win_error (err);
+	  res = error_errno_set;
+	  goto out;
+	  break;
+	case WAIT_OBJECT_0 + 1:
+	  if (!arm (read_ready))
+	    {
+	      res = error_set_errno;
+	      goto out;
+	    }
+	  break;
+	default:
+	  res = error_set_errno;
+	  goto out;
+	  break;
+	}
+      if (!duplexer && !wait (write_ready))
+	{
+	  res = error_errno_set;
+	  goto out;
+	}
+      else
+	res = success;
     }
-
-  /* If we're writing, it's a little tricky since it is possible that
-     we're attempting to open the other end of a pipe which is already
-     connected.  In that case, we detect ERROR_PIPE_BUSY, reset the
-     read_ready event and wait for the reader to allow us to connect
-     by signalling read_ready.
-
-     Once the pipe has been set up, we signal write_ready.  */
+  /* If we're writing, wait for read_ready and then connect to the
+     pipe.  This should always succeed quickly if the reader's
+     listen_client thread is running.  Then signal write_ready.  */
   if (writer)
     {
-      int err;
-      while (1)
-	if (!wait (read_ready))
-	  {
-	    res = error_errno_set;
-	    goto out;
-	  }
-	else if ((err = create_pipe (NULL, &get_io_handle (), false)) == 0)
-	  break;
-	else if (err == ERROR_PIPE_BUSY)
-	  {
-	    debug_only_printf ("pipe busy");
-	    ResetEvent (read_ready);
-	  }
-	else
-	  {
-	    debug_printf ("create of writer failed");
-	    res = error_set_errno;
-	    goto out;
-	  }
-      if (!arm (write_ready))
+      if (!wait (read_ready))
+	{
+	  res = error_errno_set;
+	  goto out;
+	}
+      else if (create_pipe (NULL, &get_io_handle (), false))
 	{
+	  debug_printf ("create of writer failed");
 	  res = error_set_errno;
 	  goto out;
 	}
+      else if (!arm (write_ready))
+	{
+	  res = error_set_errno;
+	  goto out;
+	}
+      else if (setup_overlapped () == 0)
+	res = success;
+      else
+	{
+	  debug_printf ("setup_overlapped failed, %E");
+	  res = error_set_errno;
+	}
     }
-
-  /* If setup_overlapped() succeeds (and why wouldn't it?) we are all set. */
-  if (setup_overlapped () == 0)
-    res = success;
-  else
-    {
-      debug_printf ("setup_overlapped failed, %E");
-      res = error_set_errno;
-    }
-
 out:
   if (res == error_set_errno)
     __seterrno ();
@@ -217,6 +418,8 @@ out:
 	}
       if (get_io_handle ())
 	CloseHandle (get_io_handle ());
+      if (listen_client_thr)
+	CloseHandle (listen_client_thr);
     }
   debug_printf ("res %d", res);
   return res == success;
@@ -284,37 +487,53 @@ void __reg3
 fhandler_fifo::raw_read (void *in_ptr, size_t& len)
 {
   size_t orig_len = len;
-  for (int i = 0; i < 2; i++)
+  while (1)
     {
-      fhandler_base_overlapped::raw_read (in_ptr, len);
-      if (len || i || WaitForSingleObject (read_ready, 0) != WAIT_OBJECT_0)
-	break;
-      /* If we got here, then fhandler_base_overlapped::raw_read returned 0,
-	 indicating "EOF" and something has set read_ready to zero.  That means
-	 we should have a client waiting to connect.
-	 FIXME: If the client CTRL-C's the open during this time then this
-	 could hang indefinitely.  Maybe implement a timeout?  */
-      if (!DisconnectNamedPipe (get_io_handle ()))
+      if (nconnected == 0)	/* Return EOF. */
 	{
-	  debug_printf ("DisconnectNamedPipe failed, %E");
-	  goto errno_out;
+	  len = 0;
+	  return;
 	}
-      else if (!ConnectNamedPipe (get_io_handle (), get_overlapped ())
-	       && GetLastError () != ERROR_IO_PENDING)
+      if (!check_listen_client_thread ())
+	goto errout;
+
+      for (int i = 0; i < ninstances; i++)
+	if (inst[i].connected)
+	  {
+	    ssize_t nread;
+	    len = orig_len;
+	    bool was_nonblocking = inst[i].fh->is_nonblocking ();
+	    inst[i].fh->set_nonblocking (true);
+	    inst[i].fh->fhandler_base_overlapped::raw_read (in_ptr, len);
+	    inst[i].fh->set_nonblocking (was_nonblocking);
+	    nread = (ssize_t) len;
+	    if (nread > 0)
+	      return;
+	    else if (nread < 0 && errno != EAGAIN)
+	      goto errout;
+	    else if (nread == 0 && disconnect_and_reconnect (i) < 0)
+	      goto errout;
+	  }
+      if (is_nonblocking ())
 	{
-	  debug_printf ("ConnectNamedPipe failed, %E");
-	  goto errno_out;
+	  set_errno (EAGAIN);
+	  goto errout;
+	}
+      else
+	{
+	  /* Allow interruption.  Copied from
+	     fhandler_socket_unix::open_reparse_point. */
+	  pthread_testcancel ();
+	  if (cygwait (NULL, cw_nowait, cw_sig_eintr) == WAIT_SIGNALED
+	      && !_my_tls.call_signal_handler ())
+	    {
+	      set_errno (EINTR);
+	      goto errout;
+	    }
+	  /* Don't hog the CPU. */
+	  Sleep (50);
 	}
-      else if (!arm (read_ready))
-	goto errno_out;
-      else if (!wait (get_overlapped_buffer ()->hEvent))
-	goto errout;	/* If wait() fails, errno is set so no need to set it */
-      len = orig_len;	/* Reset since raw_read above set it to zero. */
     }
-  return;
-
-errno_out:
-  __seterrno ();
 errout:
   len = -1;
 }
-- 
2.17.0


[-- Attachment #5: 0003-Cygwin-adapt-fhandler_fifo-close-to-recent-changes.patch --]
[-- Type: text/plain, Size: 1165 bytes --]

From cfdc70976e5c02f992be7bff5ad23cb067a80230 Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Thu, 17 Jan 2019 15:38:10 -0500
Subject: [PATCH FIFO, draft 3/4] Cygwin: adapt fhandler_fifo::close to recent
 changes

---
 winsup/cygwin/fhandler_fifo.cc | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 13d87eab5..5bc64408b 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -549,6 +549,21 @@ fhandler_fifo::fstatvfs (struct statvfs *sfs)
 int
 fhandler_fifo::close ()
 {
+  HANDLE evt = InterlockedExchangePointer (&lct_termination_evt, NULL);
+  HANDLE thr = InterlockedExchangePointer (&listen_client_thr, NULL);
+  if (thr)
+    {
+      if (evt)
+	SetEvent (evt);
+      WaitForSingleObject (thr, INFINITE);
+      DWORD err;
+      GetExitCodeThread (thr, &err);
+      if (err)
+	debug_printf ("listen_client_thread exited with code %d", err);
+      CloseHandle (thr);
+    }
+  if (evt)
+    CloseHandle (evt);
   CloseHandle (read_ready);
   CloseHandle (write_ready);
   return fhandler_base::close ();
-- 
2.17.0


[-- Attachment #6: 0004-Cygwin-fhandler_fifo-add-a-spinlock.patch --]
[-- Type: text/plain, Size: 3228 bytes --]

From 70e60d969c4006ddce3a4e16549d0271158ba8f4 Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Thu, 17 Jan 2019 16:55:20 -0500
Subject: [PATCH FIFO, draft 4/4] Cygwin: fhandler_fifo: add a spinlock

Don't let listen_client_thread and raw_read access the pipe_instance
list simultaneously.
---
 winsup/cygwin/fhandler.h       |  3 +++
 winsup/cygwin/fhandler_fifo.cc | 27 +++++++++++++++++++++++----
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 311cf0958..a557375f5 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1249,6 +1249,7 @@ class fhandler_fifo: public fhandler_base_overlapped
   HANDLE lct_termination_evt;
   pipe_instance inst[MAX_INSTANCES];
   int ninstances, nconnected;
+  af_unix_spinlock_t _pipe_inst_lock;
   bool __reg2 wait (HANDLE);
   char __reg2 *fifo_name (char *, const char *);
   int disconnect_and_reconnect (int);
@@ -1258,6 +1259,8 @@ class fhandler_fifo: public fhandler_base_overlapped
 public:
   fhandler_fifo ();
   DWORD listen_client_thread ();
+  void pipe_inst_lock () { _pipe_inst_lock.lock (); }
+  void pipe_inst_unlock () { _pipe_inst_lock.unlock (); }
   int open (int, mode_t);
   off_t lseek (off_t offset, int whence);
   int close ();
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 5bc64408b..452978202 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -213,8 +213,12 @@ fhandler_fifo::listen_client_thread ()
 {
   while (1)
     {
+      pipe_inst_lock ();
       if (nconnected == ninstances && create_pipe_instance () < 0)
-	return -1;
+	{
+	  pipe_inst_unlock ();
+	  return -1;
+	}
 
       /* Wait for a client to connect. */
       HANDLE w[MAX_INSTANCES + 1];
@@ -224,6 +228,7 @@ fhandler_fifo::listen_client_thread ()
 	w[i] = inst[i].connected ? inst[i].dummy_evt
 	  : inst[i].fh->get_overlapped ()->hEvent;
       w[ninstances] = lct_termination_evt;
+      pipe_inst_unlock ();
       if (!arm (read_ready))
 	goto errout;
       wait_ret = WaitForMultipleObjects (ninstances + 1, w, false, INFINITE);
@@ -234,8 +239,11 @@ fhandler_fifo::listen_client_thread ()
 	return 0;
       else
 	{
+	  pipe_inst_lock ();
 	  inst[i].connected = true;
 	  nconnected++;
+	  pipe_inst_unlock ();
+	  yield ();
 	}
     }
 errout:
@@ -497,6 +505,7 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
       if (!check_listen_client_thread ())
 	goto errout;
 
+      pipe_inst_lock ();
       for (int i = 0; i < ninstances; i++)
 	if (inst[i].connected)
 	  {
@@ -508,12 +517,22 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
 	    inst[i].fh->set_nonblocking (was_nonblocking);
 	    nread = (ssize_t) len;
 	    if (nread > 0)
-	      return;
+	      {
+		pipe_inst_unlock ();
+		return;
+	      }
 	    else if (nread < 0 && errno != EAGAIN)
-	      goto errout;
+	      {
+		pipe_inst_unlock ();
+		goto errout;
+	      }
 	    else if (nread == 0 && disconnect_and_reconnect (i) < 0)
-	      goto errout;
+	      {
+		pipe_inst_unlock ();
+		goto errout;
+	      }
 	  }
+      pipe_inst_unlock ();
       if (is_nonblocking ())
 	{
 	  set_errno (EAGAIN);
-- 
2.17.0

Re: Rewriting the FIFO code

[Corinna Vinschen]

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

On Jan 22 20:44, Ken Brown wrote:
> On 12/26/2018 2:03 PM, Corinna Vinschen wrote:
> > On Dec 26 14:00, Ken Brown wrote:
> >> Thanks for the encouragement, and Happy Holidays to you too.  One question: In
> >> the new AF_UNIX socket code you mostly used NT functions, but the existing FIFO
> >> code uses Win32 functions.  Do you prefer NT functions for new code?
> > 
> > The NT functions have some advantages over the Win32 functions.
> > For instance, WaitNamedPipe is not interruptible, while
> > NtFsControlFile(FSCTL_PIPE_WAIT) can be called asynchronously
> > and then you can just wait for an event object via cygwait
> > (see fhandler_socket_unix::wait_pipe_thread).
> > 
> > So, in theory I'd prefer NT functions, but if you feel uncomfortable
> > and just want to implement away, feel free to go ahead with Win32
> > functions.
> 
> I've decided to use Win32 functions for now, not because of discomfort but 
> because it allowed me to use more of the existing code.  I can always change 
> that later.

Sounds good to me.  Just... can we make the fifo pipes PIPE_NOWAIT, by
any chance and make fhandler_fifo a derived class of fhandler_base?

In the long run I'd like to get rid of the dreaded
fhandler_base_overlapped class.  It doesn't reflect POSIX nowait
semantics correctly and using nonblocking pipes would drop the need for
code like fhandler_base_overlapped::close.  Normal pipes should also run
nonblocking ideally.

> I've got enough working now that I think my patches could use some review.  But 
> there's still a lot more to do.  I've attached the patches, but I could send 
> them to cygwin-patches instead if you prefer.

I'd like to get 2.12.0 out of the door and then we can start pulling
your stuff, whenever you think the patches are ready for that.  In 
that case, send them to the patches list.

I reviewed the patches just visually for now, but this looks good to me
so far, except for my begging above :}


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Rewriting the FIFO code

[Ken Brown]

On 1/23/2019 8:05 AM, Corinna Vinschen wrote:
> On Jan 22 20:44, Ken Brown wrote:
>> On 12/26/2018 2:03 PM, Corinna Vinschen wrote:
>>> On Dec 26 14:00, Ken Brown wrote:
>>>> Thanks for the encouragement, and Happy Holidays to you too.  One question: In
>>>> the new AF_UNIX socket code you mostly used NT functions, but the existing FIFO
>>>> code uses Win32 functions.  Do you prefer NT functions for new code?
>>>
>>> The NT functions have some advantages over the Win32 functions.
>>> For instance, WaitNamedPipe is not interruptible, while
>>> NtFsControlFile(FSCTL_PIPE_WAIT) can be called asynchronously
>>> and then you can just wait for an event object via cygwait
>>> (see fhandler_socket_unix::wait_pipe_thread).
>>>
>>> So, in theory I'd prefer NT functions, but if you feel uncomfortable
>>> and just want to implement away, feel free to go ahead with Win32
>>> functions.
>>
>> I've decided to use Win32 functions for now, not because of discomfort but
>> because it allowed me to use more of the existing code.  I can always change
>> that later.
> 
> Sounds good to me.  Just... can we make the fifo pipes PIPE_NOWAIT, by
> any chance and make fhandler_fifo a derived class of fhandler_base?

Sure.

> In the long run I'd like to get rid of the dreaded
> fhandler_base_overlapped class.  It doesn't reflect POSIX nowait
> semantics correctly and using nonblocking pipes would drop the need for
> code like fhandler_base_overlapped::close.  Normal pipes should also run
> nonblocking ideally.
> 
>> I've got enough working now that I think my patches could use some review.  But
>> there's still a lot more to do.  I've attached the patches, but I could send
>> them to cygwin-patches instead if you prefer.
> 
> I'd like to get 2.12.0 out of the door and then we can start pulling
> your stuff, whenever you think the patches are ready for that.  In
> that case, send them to the patches list.
> 
> I reviewed the patches just visually for now, but this looks good to me
> so far, except for my begging above :}

Thanks.

Ken

Re: Rewriting the FIFO code

[Corinna Vinschen]

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

On Jan 23 15:51, Ken Brown wrote:
> On 1/23/2019 8:05 AM, Corinna Vinschen wrote:
> > On Jan 22 20:44, Ken Brown wrote:
> >> On 12/26/2018 2:03 PM, Corinna Vinschen wrote:
> >>> On Dec 26 14:00, Ken Brown wrote:
> >>>> Thanks for the encouragement, and Happy Holidays to you too.  One question: In
> >>>> the new AF_UNIX socket code you mostly used NT functions, but the existing FIFO
> >>>> code uses Win32 functions.  Do you prefer NT functions for new code?
> >>>
> >>> The NT functions have some advantages over the Win32 functions.
> >>> For instance, WaitNamedPipe is not interruptible, while
> >>> NtFsControlFile(FSCTL_PIPE_WAIT) can be called asynchronously
> >>> and then you can just wait for an event object via cygwait
> >>> (see fhandler_socket_unix::wait_pipe_thread).
> >>>
> >>> So, in theory I'd prefer NT functions, but if you feel uncomfortable
> >>> and just want to implement away, feel free to go ahead with Win32
> >>> functions.
> >>
> >> I've decided to use Win32 functions for now, not because of discomfort but
> >> because it allowed me to use more of the existing code.  I can always change
> >> that later.
> > 
> > Sounds good to me.  Just... can we make the fifo pipes PIPE_NOWAIT, by
> > any chance and make fhandler_fifo a derived class of fhandler_base?
> 
> Sure.

\o/
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Rewriting the FIFO code

[Ken Brown]

On 1/23/2019 8:05 AM, Corinna Vinschen wrote:
> On Jan 22 20:44, Ken Brown wrote:
>> On 12/26/2018 2:03 PM, Corinna Vinschen wrote:
>>> On Dec 26 14:00, Ken Brown wrote:
>>>> Thanks for the encouragement, and Happy Holidays to you too.  One question: In
>>>> the new AF_UNIX socket code you mostly used NT functions, but the existing FIFO
>>>> code uses Win32 functions.  Do you prefer NT functions for new code?
>>>
>>> The NT functions have some advantages over the Win32 functions.
>>> For instance, WaitNamedPipe is not interruptible, while
>>> NtFsControlFile(FSCTL_PIPE_WAIT) can be called asynchronously
>>> and then you can just wait for an event object via cygwait
>>> (see fhandler_socket_unix::wait_pipe_thread).
>>>
>>> So, in theory I'd prefer NT functions, but if you feel uncomfortable
>>> and just want to implement away, feel free to go ahead with Win32
>>> functions.
>>
>> I've decided to use Win32 functions for now, not because of discomfort but
>> because it allowed me to use more of the existing code.  I can always change
>> that later.
> 
> Sounds good to me.  Just... can we make the fifo pipes PIPE_NOWAIT, by
> any chance and make fhandler_fifo a derived class of fhandler_base?

I'm working on this now and have two questions.

First, if I make fhandler_fifo derived from fhandler_base, don't I have to use 
NT functions for creating pipes, etc.?  (This isn't a problem, but I just want 
to make sure I understand).  For one thing, my understanding is that 
asynchronous I/O can only be done using overlapped I/O or NT functions.  For 
another thing, fhandler_base uses NT functions for I/O, so I think I have to be 
consistent with that.

Second, fhandler_base_overlapped uses an atomic write buffer.  Is there a 
problem giving that up?  Do I need to find some other way of making sure that 
writes of PIPE_BUF bytes or fewer are atomic?

Ken

Re: Rewriting the FIFO code

[Corinna Vinschen]

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

On Feb 28 21:25, Ken Brown wrote:
> On 1/23/2019 8:05 AM, Corinna Vinschen wrote:
> > On Jan 22 20:44, Ken Brown wrote:
> >> On 12/26/2018 2:03 PM, Corinna Vinschen wrote:
> >>> On Dec 26 14:00, Ken Brown wrote:
> >>>> Thanks for the encouragement, and Happy Holidays to you too.  One question: In
> >>>> the new AF_UNIX socket code you mostly used NT functions, but the existing FIFO
> >>>> code uses Win32 functions.  Do you prefer NT functions for new code?
> >>>
> >>> The NT functions have some advantages over the Win32 functions.
> >>> For instance, WaitNamedPipe is not interruptible, while
> >>> NtFsControlFile(FSCTL_PIPE_WAIT) can be called asynchronously
> >>> and then you can just wait for an event object via cygwait
> >>> (see fhandler_socket_unix::wait_pipe_thread).
> >>>
> >>> So, in theory I'd prefer NT functions, but if you feel uncomfortable
> >>> and just want to implement away, feel free to go ahead with Win32
> >>> functions.
> >>
> >> I've decided to use Win32 functions for now, not because of discomfort but
> >> because it allowed me to use more of the existing code.  I can always change
> >> that later.
> > 
> > Sounds good to me.  Just... can we make the fifo pipes PIPE_NOWAIT, by
> > any chance and make fhandler_fifo a derived class of fhandler_base?
> 
> I'm working on this now and have two questions.
> 
> First, if I make fhandler_fifo derived from fhandler_base, don't I
> have to use NT functions for creating pipes, etc.?  (This isn't a
> problem, but I just want to make sure I understand).  For one thing,
> my understanding is that asynchronous I/O can only be done using
> overlapped I/O or NT functions.  For another thing, fhandler_base uses
> NT functions for I/O, so I think I have to be consistent with that.

For consistency it would be nice, but no, you don't have to use
NT function.  PIPE_NOWAIT is also available via Win32 API.

> Second, fhandler_base_overlapped uses an atomic write buffer.  Is
> there a problem giving that up?  Do I need to find some other way of
> making sure that writes of PIPE_BUF bytes or fewer are atomic?

You can use NOWAIT pipes atomically.  MSDN has a failry nice
description (see under "Wait Mode":

https://docs.microsoft.com/en-us/windows/desktop/ipc/named-pipe-type-read-and-wait-modes


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Rewriting the FIFO code

[Ken Brown]

On 2/28/2019 5:23 PM, Corinna Vinschen wrote:
> On Feb 28 21:25, Ken Brown wrote:
>> On 1/23/2019 8:05 AM, Corinna Vinschen wrote:
>>> On Jan 22 20:44, Ken Brown wrote:
>>>> On 12/26/2018 2:03 PM, Corinna Vinschen wrote:
>>>>> On Dec 26 14:00, Ken Brown wrote:
>>>>>> Thanks for the encouragement, and Happy Holidays to you too.  One question: In
>>>>>> the new AF_UNIX socket code you mostly used NT functions, but the existing FIFO
>>>>>> code uses Win32 functions.  Do you prefer NT functions for new code?
>>>>>
>>>>> The NT functions have some advantages over the Win32 functions.
>>>>> For instance, WaitNamedPipe is not interruptible, while
>>>>> NtFsControlFile(FSCTL_PIPE_WAIT) can be called asynchronously
>>>>> and then you can just wait for an event object via cygwait
>>>>> (see fhandler_socket_unix::wait_pipe_thread).
>>>>>
>>>>> So, in theory I'd prefer NT functions, but if you feel uncomfortable
>>>>> and just want to implement away, feel free to go ahead with Win32
>>>>> functions.
>>>>
>>>> I've decided to use Win32 functions for now, not because of discomfort but
>>>> because it allowed me to use more of the existing code.  I can always change
>>>> that later.
>>>
>>> Sounds good to me.  Just... can we make the fifo pipes PIPE_NOWAIT, by
>>> any chance and make fhandler_fifo a derived class of fhandler_base?
>>
>> I'm working on this now and have two questions.
>>
>> First, if I make fhandler_fifo derived from fhandler_base, don't I
>> have to use NT functions for creating pipes, etc.?  (This isn't a
>> problem, but I just want to make sure I understand).  For one thing,
>> my understanding is that asynchronous I/O can only be done using
>> overlapped I/O or NT functions.  For another thing, fhandler_base uses
>> NT functions for I/O, so I think I have to be consistent with that.
> 
> For consistency it would be nice, but no, you don't have to use
> NT function.  PIPE_NOWAIT is also available via Win32 API.

I've finally finished a first pass at the FIFO code.  I ended up finding it more 
convenient to use the NT API and to initially create pipes in blocking mode, so 
that I could easily wait for a client to connect.  After there's a connection, I 
set the pipes non-blocking.

I've tested the code by running Kerrisk's server/client programs cited in 
https://cygwin.com/ml/cygwin/2015-03/msg00047.html.  I've also run the test case 
that I posted in http://www.cygwin.org/ml/cygwin/2015-12/msg00311.html.

There's still a lot more testing that needs to be done, and I haven't thought at 
all about the duplexer case yet, but I think I've done enough that a review 
would be helpful when you get the time.  I'll send the patches to cygwin-patches 
shortly.

Once we finish the review/revise cycle, it might make sense to commit the 
patches to a topic/fifo branch for further work.  I don't think it's ready for 
master yet.

Ken

Re: Rewriting the FIFO code

[Corinna Vinschen]

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

On Mar 22 19:27, Ken Brown wrote:
> On 2/28/2019 5:23 PM, Corinna Vinschen wrote:
> > For consistency it would be nice, but no, you don't have to use
> > NT function.  PIPE_NOWAIT is also available via Win32 API.
> 
> I've finally finished a first pass at the FIFO code.  I ended up
> finding it more convenient to use the NT API and to initially create
> pipes in blocking mode, so that I could easily wait for a client to
> connect.  After there's a connection, I set the pipes non-blocking.

Sure, if that helps.

> I've tested the code by running Kerrisk's server/client programs cited
> in https://cygwin.com/ml/cygwin/2015-03/msg00047.html.  I've also run
> the test case that I posted in
> http://www.cygwin.org/ml/cygwin/2015-12/msg00311.html.
> 
> There's still a lot more testing that needs to be done, and I haven't
> thought at all about the duplexer case yet, but I think I've done
> enough that a review would be helpful when you get the time.  I'll
> send the patches to cygwin-patches shortly.

Seen them, thank you.  Not sure how much time I have over the weekend,
but a first scan of your patch looks good.

> Once we finish the review/revise cycle, it might make sense to commit
> the patches to a topic/fifo branch for further work.  I don't think
> it's ready for master yet.

Sounds like a plan!

Here's a question: Even if you think this isn't ready for prime time,
how much of the *old* FIFO implementation does your new code cover?
90%?  100%?  If the code isn't quite finished from your POV, but it
already covers all scenarios the old code (barely) worked, what
speaks against making this a 3.1?


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Rewriting the FIFO code

[Ken Brown]

On 3/23/2019 6:23 AM, Corinna Vinschen wrote:
> On Mar 22 19:27, Ken Brown wrote:
>> On 2/28/2019 5:23 PM, Corinna Vinschen wrote:
>>> For consistency it would be nice, but no, you don't have to use
>>> NT function.  PIPE_NOWAIT is also available via Win32 API.
>>
>> I've finally finished a first pass at the FIFO code.  I ended up
>> finding it more convenient to use the NT API and to initially create
>> pipes in blocking mode, so that I could easily wait for a client to
>> connect.  After there's a connection, I set the pipes non-blocking.
> 
> Sure, if that helps.
> 
>> I've tested the code by running Kerrisk's server/client programs cited
>> in https://cygwin.com/ml/cygwin/2015-03/msg00047.html.  I've also run
>> the test case that I posted in
>> http://www.cygwin.org/ml/cygwin/2015-12/msg00311.html.
>>
>> There's still a lot more testing that needs to be done, and I haven't
>> thought at all about the duplexer case yet, but I think I've done
>> enough that a review would be helpful when you get the time.  I'll
>> send the patches to cygwin-patches shortly.
> 
> Seen them, thank you.  Not sure how much time I have over the weekend,
> but a first scan of your patch looks good.
> 
>> Once we finish the review/revise cycle, it might make sense to commit
>> the patches to a topic/fifo branch for further work.  I don't think
>> it's ready for master yet.
> 
> Sounds like a plan!
> 
> Here's a question: Even if you think this isn't ready for prime time,
> how much of the *old* FIFO implementation does your new code cover?
> 90%?  100%?  If the code isn't quite finished from your POV, but it
> already covers all scenarios the old code (barely) worked, what
> speaks against making this a 3.1?

I'm not worried about coverage; the only thing missing that the old code covered 
is the duplex case, which probably won't be difficult.

My main concern is testing.  I haven't yet tested the select or fork code.  But 
if that works OK, then maybe this will be ready for 3.1.

Ken

Re: Rewriting the FIFO code

[Corinna Vinschen]

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

On Mar 23 15:23, Ken Brown wrote:
> On 3/23/2019 6:23 AM, Corinna Vinschen wrote:
> > On Mar 22 19:27, Ken Brown wrote:
> >> On 2/28/2019 5:23 PM, Corinna Vinschen wrote:
> >>> For consistency it would be nice, but no, you don't have to use
> >>> NT function.  PIPE_NOWAIT is also available via Win32 API.
> >>
> >> I've finally finished a first pass at the FIFO code.  I ended up
> >> finding it more convenient to use the NT API and to initially create
> >> pipes in blocking mode, so that I could easily wait for a client to
> >> connect.  After there's a connection, I set the pipes non-blocking.
> > 
> > Sure, if that helps.
> > 
> >> I've tested the code by running Kerrisk's server/client programs cited
> >> in https://cygwin.com/ml/cygwin/2015-03/msg00047.html.  I've also run
> >> the test case that I posted in
> >> http://www.cygwin.org/ml/cygwin/2015-12/msg00311.html.
> >>
> >> There's still a lot more testing that needs to be done, and I haven't
> >> thought at all about the duplexer case yet, but I think I've done
> >> enough that a review would be helpful when you get the time.  I'll
> >> send the patches to cygwin-patches shortly.
> > 
> > Seen them, thank you.  Not sure how much time I have over the weekend,
> > but a first scan of your patch looks good.
> > 
> >> Once we finish the review/revise cycle, it might make sense to commit
> >> the patches to a topic/fifo branch for further work.  I don't think
> >> it's ready for master yet.
> > 
> > Sounds like a plan!
> > 
> > Here's a question: Even if you think this isn't ready for prime time,
> > how much of the *old* FIFO implementation does your new code cover?
> > 90%?  100%?  If the code isn't quite finished from your POV, but it
> > already covers all scenarios the old code (barely) worked, what
> > speaks against making this a 3.1?
> 
> I'm not worried about coverage; the only thing missing that the old
> code covered is the duplex case, which probably won't be difficult.
> 
> My main concern is testing.  I haven't yet tested the select or fork
> code.  But if that works OK, then maybe this will be ready for 3.1.

I have no specific plans for 3.1 so far, with 3.0 already covering more
stuff than I originally anticipated.  We can easily define 3.1 as the
new FIFO release and release it in our own time.


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]