Cygwin: FIFO: add a timeout to take_ownership

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.
This commit is contained in:
Ken Brown 2020-08-03 09:17:06 -04:00
parent 6acce025d0
commit 6ed067a0ae
3 changed files with 30 additions and 53 deletions

View File

@ -1487,7 +1487,7 @@ public:
void fifo_client_lock () { _fifo_client_lock.lock (); } void fifo_client_lock () { _fifo_client_lock.lock (); }
void fifo_client_unlock () { _fifo_client_lock.unlock (); } 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_lock () { shmem->reading_lock (); }
void reading_unlock () { shmem->reading_unlock (); } void reading_unlock () { shmem->reading_unlock (); }

View File

@ -1214,16 +1214,17 @@ fhandler_fifo::raw_write (const void *ptr, size_t len)
return ret; return ret;
} }
/* Called from raw_read and select.cc:peek_fifo. Return WAIT_OBJECT_0 /* Called from raw_read and select.cc:peek_fifo. */
on success. */ int
DWORD fhandler_fifo::take_ownership (DWORD timeout)
fhandler_fifo::take_ownership ()
{ {
int ret = 0;
owner_lock (); owner_lock ();
if (get_owner () == me) if (get_owner () == me)
{ {
owner_unlock (); owner_unlock ();
return WAIT_OBJECT_0; return 0;
} }
set_pending_owner (me); set_pending_owner (me);
/* Wake up my fifo_reader_thread. */ /* Wake up my fifo_reader_thread. */
@ -1233,18 +1234,25 @@ fhandler_fifo::take_ownership ()
SetEvent (update_needed_evt); SetEvent (update_needed_evt);
owner_unlock (); owner_unlock ();
/* The reader threads should now do the transfer. */ /* The reader threads should now do the transfer. */
DWORD waitret = cygwait (owner_found_evt, cw_cancel | cw_sig_eintr); switch (WaitForSingleObject (owner_found_evt, timeout))
owner_lock ();
if (waitret == WAIT_OBJECT_0
&& (get_owner () != me || get_pending_owner ()))
{ {
/* Something went wrong. Return WAIT_TIMEOUT, which can't be case WAIT_OBJECT_0:
returned by the above cygwait call. */ owner_lock ();
set_pending_owner (null_fr_id); if (get_owner () != me)
waitret = WAIT_TIMEOUT; {
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 ret;
return waitret;
} }
void __reg3 void __reg3
@ -1255,38 +1263,12 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
while (1) while (1)
{ {
int nconnected = 0;
/* No one else can take ownership while we hold the reading_lock. */ /* No one else can take ownership while we hold the reading_lock. */
reading_lock (); reading_lock ();
switch (take_ownership ()) if (take_ownership (10) < 0)
{ goto maybe_retry;
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;
}
/* Poll the connected clients for input. Make two passes. On /* Poll the connected clients for input. Make two passes. On
the first pass, just try to read from the client from which 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. */ /* Second pass. */
int nconnected = 0;
for (int i = 0; i < nhandlers; i++) for (int i = 0; i < nhandlers; i++)
if (fc_handler[i].state >= fc_closing) if (fc_handler[i].state >= fc_closing)
{ {
@ -1375,6 +1356,7 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
len = 0; len = 0;
return; return;
} }
maybe_retry:
reading_unlock (); reading_unlock ();
if (is_nonblocking ()) if (is_nonblocking ())
{ {

View File

@ -867,16 +867,11 @@ peek_fifo (select_record *s, bool from_select)
} }
fh->reading_lock (); 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 (); fh->reading_unlock ();
gotone += s->read_ready = true;
if (s->except_selected)
gotone += s->except_ready = true;
goto out; goto out;
} }
fh->fifo_client_lock (); fh->fifo_client_lock ();
int nconnected = 0; int nconnected = 0;
for (int i = 0; i < fh->get_nhandlers (); i++) for (int i = 0; i < fh->get_nhandlers (); i++)