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
