public inbox for cygwin-patches@cygwin.com
 help / color / mirror / Atom feed
From: Ken Brown <kbrown@cornell.edu>
To: cygwin-patches@cygwin.com
Subject: [PATCH 3/8] Cygwin: FIFO: add a timeout to take_ownership
Date: Tue,  4 Aug 2020 08:55:02 -0400	[thread overview]
Message-ID: <20200804125507.8842-4-kbrown@cornell.edu> (raw)
In-Reply-To: <20200804125507.8842-1-kbrown@cornell.edu>

fhandler_fifo::take_ownership() is called from select.cc::peek_fifo
and fhandler_fifo::raw_read and could potentially block indefinitely
if something goes wrong.  This is always undesirable in peek_fifo, and
it is undesirable in a nonblocking read.  Fix this by adding a timeout
parameter to take_ownership.

Arbitrarily use a 1 ms timeout in peek_fifo and a 10 ms timeout in
raw_read.  These numbers may have to be tweaked based on experience.

Replace the call to cygwait in take_ownership by a call to WFSO.
There's no need to allow interruption now that we have a timeout.
---
 winsup/cygwin/fhandler.h       |  2 +-
 winsup/cygwin/fhandler_fifo.cc | 74 +++++++++++++---------------------
 winsup/cygwin/select.cc        |  7 +---
 3 files changed, 30 insertions(+), 53 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 60bd27e00..5488327a2 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1487,7 +1487,7 @@ public:
   void fifo_client_lock () { _fifo_client_lock.lock (); }
   void fifo_client_unlock () { _fifo_client_lock.unlock (); }
 
-  DWORD take_ownership ();
+  int take_ownership (DWORD timeout = INFINITE);
   void reading_lock () { shmem->reading_lock (); }
   void reading_unlock () { shmem->reading_unlock (); }
 
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 7b87aab00..b8a47f27f 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -1214,16 +1214,17 @@ fhandler_fifo::raw_write (const void *ptr, size_t len)
   return ret;
 }
 
-/* Called from raw_read and select.cc:peek_fifo.  Return WAIT_OBJECT_0
-   on success.  */
-DWORD
-fhandler_fifo::take_ownership ()
+/* Called from raw_read and select.cc:peek_fifo. */
+int
+fhandler_fifo::take_ownership (DWORD timeout)
 {
+  int ret = 0;
+
   owner_lock ();
   if (get_owner () == me)
     {
       owner_unlock ();
-      return WAIT_OBJECT_0;
+      return 0;
     }
   set_pending_owner (me);
   /* Wake up my fifo_reader_thread. */
@@ -1233,18 +1234,25 @@ fhandler_fifo::take_ownership ()
     SetEvent (update_needed_evt);
   owner_unlock ();
   /* The reader threads should now do the transfer. */
-  DWORD waitret = cygwait (owner_found_evt, cw_cancel | cw_sig_eintr);
-  owner_lock ();
-  if (waitret == WAIT_OBJECT_0
-      && (get_owner () != me || get_pending_owner ()))
+  switch (WaitForSingleObject (owner_found_evt, timeout))
     {
-      /* Something went wrong.  Return WAIT_TIMEOUT, which can't be
-	 returned by the above cygwait call. */
-      set_pending_owner (null_fr_id);
-      waitret = WAIT_TIMEOUT;
+    case WAIT_OBJECT_0:
+      owner_lock ();
+      if (get_owner () != me)
+	{
+	  debug_printf ("owner_found_evt signaled, but I'm not the owner");
+	  ret = -1;
+	}
+      owner_unlock ();
+      break;
+    case WAIT_TIMEOUT:
+      debug_printf ("timed out");
+      ret = -1;
+    default:
+      debug_printf ("WFSO failed, %E");
+      ret = -1;
     }
-  owner_unlock ();
-  return waitret;
+  return ret;
 }
 
 void __reg3
@@ -1255,38 +1263,12 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
 
   while (1)
     {
+      int nconnected = 0;
+
       /* No one else can take ownership while we hold the reading_lock. */
       reading_lock ();
-      switch (take_ownership ())
-	{
-	case WAIT_OBJECT_0:
-	  break;
-	case WAIT_SIGNALED:
-	  if (_my_tls.call_signal_handler ())
-	    {
-	      reading_unlock ();
-	      continue;
-	    }
-	  else
-	    {
-	      set_errno (EINTR);
-	      reading_unlock ();
-	      goto errout;
-	    }
-	  break;
-	case WAIT_CANCELED:
-	  reading_unlock ();
-	  pthread::static_cancel_self ();
-	  break;
-	case WAIT_TIMEOUT:
-	  reading_unlock ();
-	  debug_printf ("take_ownership returned an unexpected result; retry");
-	  continue;
-	default:
-	  reading_unlock ();
-	  debug_printf ("unknown error while trying to take ownership, %E");
-	  goto errout;
-	}
+      if (take_ownership (10) < 0)
+	goto maybe_retry;
 
       /* Poll the connected clients for input.  Make two passes.  On
 	 the first pass, just try to read from the client from which
@@ -1332,7 +1314,6 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
 	}
 
       /* Second pass. */
-      int nconnected = 0;
       for (int i = 0; i < nhandlers; i++)
 	if (fc_handler[i].state >= fc_closing)
 	  {
@@ -1375,6 +1356,7 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
 	  len = 0;
 	  return;
 	}
+maybe_retry:
       reading_unlock ();
       if (is_nonblocking ())
 	{
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 3f3f33fb5..1ba76c817 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -867,16 +867,11 @@ peek_fifo (select_record *s, bool from_select)
 	}
 
       fh->reading_lock ();
-      if (fh->take_ownership () != WAIT_OBJECT_0)
+      if (fh->take_ownership (1) < 0)
 	{
-	  select_printf ("%s, unable to take ownership", fh->get_name ());
 	  fh->reading_unlock ();
-	  gotone += s->read_ready = true;
-	  if (s->except_selected)
-	    gotone += s->except_ready = true;
 	  goto out;
 	}
-
       fh->fifo_client_lock ();
       int nconnected = 0;
       for (int i = 0; i < fh->get_nhandlers (); i++)
-- 
2.28.0


  parent reply	other threads:[~2020-08-04 12:55 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-04 12:54 [PATCH 0/8] FIFO: bug fixes and small improvements Ken Brown
2020-08-04 12:55 ` [PATCH 1/8] Cygwin: FIFO: lock fixes Ken Brown
2020-08-04 12:55 ` [PATCH 2/8] Cygwin: FIFO: fix timing issue with owner change Ken Brown
2020-08-04 12:55 ` Ken Brown [this message]
2020-08-04 12:55 ` [PATCH 4/8] Cygwin: FIFO: reorganize some fifo_client_handler methods Ken Brown
2020-08-04 12:55 ` [PATCH 5/8] Cygwin: FIFO: don't read from pipes that are closing Ken Brown
2020-08-04 12:55 ` [PATCH 6/8] Cygwin: FIFO: synchronize the fifo_reader and fifosel threads Ken Brown
2020-08-04 12:55 ` [PATCH 7/8] Cygwin: FIFO: fix indentation Ken Brown
2020-08-04 12:55 ` [PATCH 8/8] Cygwin: FIFO: add a third pass to raw_read Ken Brown
2020-08-04 15:04 ` [PATCH 0/8] FIFO: bug fixes and small improvements Corinna Vinschen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200804125507.8842-4-kbrown@cornell.edu \
    --to=kbrown@cornell.edu \
    --cc=cygwin-patches@cygwin.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).