Cygwin: FIFO: fix problems finding new owner

When the owning reader closes and there are still readers open, the
owner needs to wait for a new owner to be found before closing its
fifo_client handlers.  This involves a loop in which dec_nreaders is
called at the beginning and inc_nreaders is called at the end.  Any
other reader that tries to access shmem->_nreaders during this loop
will therefore get an inaccurate answer.

Fix this by adding an nreaders method and using it instead of
dec_nreaders and inc_nreaders.  Also add nreaders_lock to control
access to the shmem->_nreaders.

Make various other changes to improve the reliability of finding a new
owner.

winsup/cygwin/fhandler.h

@@ -1334,7 +1334,7 @@ class fifo_shmem_t
 {
   LONG _nreaders;
   fifo_reader_id_t _owner, _prev_owner, _pending_owner;
-  af_unix_spinlock_t _owner_lock, _reading_lock, _reader_opening_lock;
+  af_unix_spinlock_t _owner_lock, _reading_lock, _reader_opening_lock, _nreaders_lock;
 
   /* Info about shared memory block used for temporary storage of the
      owner's fc_handler list. */
@@ -1342,6 +1342,7 @@ class fifo_shmem_t
     _sh_fc_handler_updated;
 
 public:
+  int nreaders () const { return (int) _nreaders; }
   int inc_nreaders () { return (int) InterlockedIncrement (&_nreaders); }
   int dec_nreaders () { return (int) InterlockedDecrement (&_nreaders); }
 
@@ -1358,6 +1359,8 @@ public:
   void reading_unlock () { _reading_lock.unlock (); }
   void reader_opening_lock () { _reader_opening_lock.lock (); }
   void reader_opening_unlock () { _reader_opening_lock.unlock (); }
+  void nreaders_lock () { _nreaders_lock.lock (); }
+  void nreaders_unlock () { _nreaders_lock.unlock (); }
 
   int get_shared_nhandlers () const { return (int) _sh_nhandlers; }
   void set_shared_nhandlers (int n) { InterlockedExchange (&_sh_nhandlers, n); }
@@ -1426,8 +1429,11 @@ class fhandler_fifo: public fhandler_base
   int reopen_shared_fc_handler ();
   int remap_shared_fc_handler (size_t);
 
+  int nreaders () const { return shmem->nreaders (); }
   int inc_nreaders () { return shmem->inc_nreaders (); }
   int dec_nreaders () { return shmem->dec_nreaders (); }
+  void nreaders_lock () { shmem->nreaders_lock (); }
+  void nreaders_unlock () { shmem->nreaders_unlock (); }
 
   fifo_reader_id_t get_prev_owner () const { return shmem->get_prev_owner (); }
   void set_prev_owner (fifo_reader_id_t fr_id)

winsup/cygwin/fhandler_fifo.cc

@@ -371,6 +371,8 @@ fhandler_fifo::record_connection (fifo_client_handler& fc,
 int
 fhandler_fifo::update_my_handlers ()
 {
+  int ret = 0;
+
   close_all_handlers ();
   fifo_reader_id_t prev = get_prev_owner ();
   if (!prev)
@@ -387,7 +389,7 @@ fhandler_fifo::update_my_handlers ()
     {
       debug_printf ("Can't open process of previous owner, %E");
       __seterrno ();
-      return -1;
+      goto out;
     }
 
   for (int i = 0; i < get_shared_nhandlers (); i++)
@@ -402,11 +404,13 @@ fhandler_fifo::update_my_handlers ()
 	  debug_printf ("Can't duplicate handle of previous owner, %E");
 	  --nhandlers;
 	  __seterrno ();
-	  return -1;
+	  goto out;
 	}
       fc.state = shared_fc_handler[i].state;
     }
-  return 0;
+out:
+  set_prev_owner (null_fr_id);
+  return ret;
 }
 
 int
@@ -1414,41 +1418,59 @@ fhandler_fifo::close ()
 {
   if (reader)
     {
-      /* If we're the owner, try to find a new owner. */
+      /* If we're the owner, we can't close our fc_handlers if a new
-      bool find_new_owner = false;
+	 owner might need to duplicate them. */
+      bool close_fc_ok = false;
 
       cancel_reader_thread ();
-      owner_lock ();
+      nreaders_lock ();
-      if (get_owner () == me)
-	{
-	  find_new_owner = true;
-	  set_owner (null_fr_id);
-	  set_prev_owner (me);
-	  owner_needed ();
-	}
-      owner_unlock ();
       if (dec_nreaders () == 0)
-	ResetEvent (read_ready);
-      else if (find_new_owner && !IsEventSignalled (owner_found_evt))
 	{
-	  bool found = false;
+	  close_fc_ok = true;
-	  do
+	  ResetEvent (read_ready);
-	    if (dec_nreaders () >= 0)
+	  ResetEvent (owner_needed_evt);
-	      {
+	  ResetEvent (owner_found_evt);
-		/* There's still another reader open. */
+	  set_owner (null_fr_id);
-		if (WaitForSingleObject (owner_found_evt, 1) == WAIT_OBJECT_0)
+	  set_prev_owner (null_fr_id);
-		  found = true;
+	  set_pending_owner (null_fr_id);
-		else
+	  set_shared_nhandlers (0);
-		  {
+	}
-		    owner_lock ();
+      else
-		    if (get_owner ()) /* We missed owner_found_evt? */
+	{
-		      found = true;
+	  owner_lock ();
-		    else
+	  if (get_owner () != me)
-		      owner_needed ();
+	    close_fc_ok = true;
-		    owner_unlock ();
+	  else
-		  }
+	    {
-	      }
+	      set_owner (null_fr_id);
-	  while (inc_nreaders () > 0 && !found);
+	      set_prev_owner (me);
+	      if (!get_pending_owner ())
+		owner_needed ();
+	    }
+	  owner_unlock ();
+	}
+      nreaders_unlock ();
+      while (!close_fc_ok)
+	{
+	  if (WaitForSingleObject (owner_found_evt, 1) == WAIT_OBJECT_0)
+	    close_fc_ok = true;
+	  else
+	    {
+	      nreaders_lock ();
+	      if (!nreaders ())
+		{
+		  close_fc_ok = true;
+		  nreaders_unlock ();
+		}
+	      else
+		{
+		  nreaders_unlock ();
+		  owner_lock ();
+		  if (get_owner () || get_prev_owner () != me)
+		    close_fc_ok = true;
+		  owner_unlock ();
+		}
+	    }
 	}
       close_all_handlers ();
       if (fc_handler)