* [PATCH 0/8] FIFO: bug fixes and small improvements
@ 2020-08-04 12:54 Ken Brown
2020-08-04 12:55 ` [PATCH 1/8] Cygwin: FIFO: lock fixes Ken Brown
` (8 more replies)
0 siblings, 9 replies; 10+ messages in thread
From: Ken Brown @ 2020-08-04 12:54 UTC (permalink / raw)
To: cygwin-patches
The second patch in this series fixes a serious bug that has resulted
in observable hangs. The other patches are minor bug fixes or minor
performance improvements.
Ken Brown (8):
Cygwin: FIFO: lock fixes
Cygwin: FIFO: fix timing issue with owner change
Cygwin: FIFO: add a timeout to take_ownership
Cygwin: FIFO: reorganize some fifo_client_handler methods
Cygwin: FIFO: don't read from pipes that are closing
Cygwin: FIFO: synchronize the fifo_reader and fifosel threads
Cygwin: FIFO: fix indentation
Cygwin: FIFO: add a third pass to raw_read
winsup/cygwin/fhandler.h | 24 +--
winsup/cygwin/fhandler_fifo.cc | 358 ++++++++++++++++++++++-----------
winsup/cygwin/select.cc | 38 ++--
3 files changed, 268 insertions(+), 152 deletions(-)
--
2.28.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/8] Cygwin: FIFO: lock fixes
2020-08-04 12:54 [PATCH 0/8] FIFO: bug fixes and small improvements Ken Brown
@ 2020-08-04 12:55 ` Ken Brown
2020-08-04 12:55 ` [PATCH 2/8] Cygwin: FIFO: fix timing issue with owner change Ken Brown
` (7 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Ken Brown @ 2020-08-04 12:55 UTC (permalink / raw)
To: cygwin-patches
Add some missing locks and remove one extra unlock. Clarify for some
functions whether caller or callee acquires lock, and add appropriate
comments.
---
winsup/cygwin/fhandler_fifo.cc | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index e9d0187d4..ee7f47c0c 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -316,6 +316,7 @@ fhandler_fifo::wait_open_pipe (HANDLE& ph)
return status;
}
+/* Always called with fifo_client_lock in place. */
int
fhandler_fifo::add_client_handler (bool new_pipe_instance)
{
@@ -345,6 +346,7 @@ fhandler_fifo::add_client_handler (bool new_pipe_instance)
return 0;
}
+/* Always called with fifo_client_lock in place. */
void
fhandler_fifo::delete_client_handler (int i)
{
@@ -354,7 +356,8 @@ fhandler_fifo::delete_client_handler (int i)
(nhandlers - i) * sizeof (fc_handler[i]));
}
-/* Delete handlers that we will never read from. */
+/* Delete handlers that we will never read from. Always called with
+ fifo_client_lock in place. */
void
fhandler_fifo::cleanup_handlers ()
{
@@ -369,6 +372,7 @@ fhandler_fifo::cleanup_handlers ()
}
}
+/* Always called with fifo_client_lock in place. */
void
fhandler_fifo::record_connection (fifo_client_handler& fc,
fifo_client_connect_state s)
@@ -398,6 +402,7 @@ fhandler_fifo::update_my_handlers ()
if (!prev_proc)
api_fatal ("Can't open process of previous owner, %E");
+ fifo_client_lock ();
for (int i = 0; i < get_shared_nhandlers (); i++)
{
if (add_client_handler (false) < 0)
@@ -419,10 +424,12 @@ fhandler_fifo::update_my_handlers ()
fc.last_read = shared_fc_handler[i].last_read;
}
}
+ fifo_client_unlock ();
set_prev_owner (null_fr_id);
return ret;
}
+/* Always called with fifo_client_lock and owner_lock in place. */
int
fhandler_fifo::update_shared_handlers ()
{
@@ -435,9 +442,7 @@ fhandler_fifo::update_shared_handlers ()
set_shared_nhandlers (nhandlers);
memcpy (shared_fc_handler, fc_handler, nhandlers * sizeof (fc_handler[0]));
shared_fc_handler_updated (true);
- owner_lock ();
set_prev_owner (me);
- owner_unlock ();
return 0;
}
@@ -509,7 +514,6 @@ fhandler_fifo::fifo_reader_thread_func ()
if (update_my_handlers () < 0)
debug_printf ("error updating my handlers, %E");
owner_found ();
- owner_unlock ();
/* Fall through to owner_listen. */
}
}
@@ -602,8 +606,13 @@ owner_listen:
}
if (ph)
NtClose (ph);
- if (update && update_shared_handlers () < 0)
- api_fatal ("Can't update shared handlers, %E");
+ if (update)
+ {
+ owner_lock ();
+ if (get_owner () == me && update_shared_handlers () < 0)
+ api_fatal ("Can't update shared handlers, %E");
+ owner_unlock ();
+ }
fifo_client_unlock ();
if (cancel)
goto canceled;
@@ -1402,9 +1411,11 @@ fhandler_fifo::fstatvfs (struct statvfs *sfs)
void
fhandler_fifo::close_all_handlers ()
{
+ fifo_client_lock ();
for (int i = 0; i < nhandlers; i++)
fc_handler[i].close ();
nhandlers = 0;
+ fifo_client_unlock ();
}
fifo_client_connect_state
--
2.28.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/8] Cygwin: FIFO: fix timing issue with owner change
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 ` Ken Brown
2020-08-04 12:55 ` [PATCH 3/8] Cygwin: FIFO: add a timeout to take_ownership Ken Brown
` (6 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Ken Brown @ 2020-08-04 12:55 UTC (permalink / raw)
To: cygwin-patches
fhandler_fifo::take_ownership() tacitly assumes that the current
owner's fifo_reader_thread will be woken up from WFMO when
update_needed_evt is signaled. But it's possible that the the current
owner's fifo_reader_thread is at the beginning of its main loop rather
than in its WFMO call when that event is signaled.
In this case the owner will never see that the event has been
signaled, and it will never update the shared fifo_client_handlers.
The reader that wants to take ownership will then spin its wheels
forever.
Fix this by having the current owner call update_shared_handlers at
the beginning of its loop, if necessary.
---
winsup/cygwin/fhandler_fifo.cc | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index ee7f47c0c..7b87aab00 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -472,17 +472,34 @@ fhandler_fifo::fifo_reader_thread_func ()
if (pending_owner)
{
- if (pending_owner != me)
+ if (pending_owner == me)
+ take_ownership = true;
+ else if (cur_owner != me)
idle = true;
else
- take_ownership = true;
+ {
+ /* I'm the owner but someone else wants to be. Have I
+ already seen and reacted to update_needed_evt? */
+ if (WaitForSingleObject (update_needed_evt, 0) == WAIT_OBJECT_0)
+ {
+ /* No, I haven't. */
+ fifo_client_lock ();
+ if (update_shared_handlers () < 0)
+ api_fatal ("Can't update shared handlers, %E");
+ fifo_client_unlock ();
+ }
+ owner_unlock ();
+ /* Yield to pending owner. */
+ Sleep (1);
+ continue;
+ }
}
else if (!cur_owner)
take_ownership = true;
else if (cur_owner != me)
idle = true;
else
- /* I'm the owner. */
+ /* I'm the owner and there's no pending owner. */
goto owner_listen;
if (idle)
{
@@ -1212,7 +1229,7 @@ fhandler_fifo::take_ownership ()
/* Wake up my fifo_reader_thread. */
owner_needed ();
if (get_owner ())
- /* Wake up owner's fifo_reader_thread. */
+ /* Wake up the owner and request an update of the shared fc_handlers. */
SetEvent (update_needed_evt);
owner_unlock ();
/* The reader threads should now do the transfer. */
--
2.28.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/8] Cygwin: FIFO: add a timeout to take_ownership
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
2020-08-04 12:55 ` [PATCH 4/8] Cygwin: FIFO: reorganize some fifo_client_handler methods Ken Brown
` (5 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Ken Brown @ 2020-08-04 12:55 UTC (permalink / raw)
To: cygwin-patches
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 4/8] Cygwin: FIFO: reorganize some fifo_client_handler methods
2020-08-04 12:54 [PATCH 0/8] FIFO: bug fixes and small improvements Ken Brown
` (2 preceding siblings ...)
2020-08-04 12:55 ` [PATCH 3/8] Cygwin: FIFO: add a timeout to take_ownership Ken Brown
@ 2020-08-04 12:55 ` Ken Brown
2020-08-04 12:55 ` [PATCH 5/8] Cygwin: FIFO: don't read from pipes that are closing Ken Brown
` (4 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Ken Brown @ 2020-08-04 12:55 UTC (permalink / raw)
To: cygwin-patches
Rename the existing set_state() to query_and_set_state() to reflect
what it really does. (It queries the O/S for the pipe state.) Add a
new set_state() method, which is a standard setter, and a
corresponding getter get_state().
---
winsup/cygwin/fhandler.h | 9 ++++--
winsup/cygwin/fhandler_fifo.cc | 50 +++++++++++++++++++---------------
winsup/cygwin/select.cc | 28 +++++++++++--------
3 files changed, 50 insertions(+), 37 deletions(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 5488327a2..f64eabda4 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1297,11 +1297,14 @@ enum fifo_client_connect_state
struct fifo_client_handler
{
HANDLE h;
- fifo_client_connect_state state;
+ fifo_client_connect_state _state;
bool last_read; /* true if our last successful read was from this client. */
- fifo_client_handler () : h (NULL), state (fc_unknown), last_read (false) {}
+ fifo_client_handler () : h (NULL), _state (fc_unknown), last_read (false) {}
void close () { NtClose (h); }
- fifo_client_connect_state set_state ();
+ fifo_client_connect_state get_state () const { return _state; }
+ void set_state (fifo_client_connect_state s) { _state = s; }
+ /* Query O/S. Return previous state. */
+ fifo_client_connect_state query_and_set_state ();
};
class fhandler_fifo;
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index b8a47f27f..c816c692a 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -340,7 +340,7 @@ fhandler_fifo::add_client_handler (bool new_pipe_instance)
if (!ph)
return -1;
fc.h = ph;
- fc.state = fc_listening;
+ fc.set_state (fc_listening);
}
fc_handler[nhandlers++] = fc;
return 0;
@@ -365,7 +365,7 @@ fhandler_fifo::cleanup_handlers ()
while (i < nhandlers)
{
- if (fc_handler[i].state < fc_closing)
+ if (fc_handler[i].get_state () < fc_closing)
delete_client_handler (i);
else
i++;
@@ -377,7 +377,7 @@ void
fhandler_fifo::record_connection (fifo_client_handler& fc,
fifo_client_connect_state s)
{
- fc.state = s;
+ fc.set_state (s);
set_pipe_non_blocking (fc.h, true);
}
@@ -414,13 +414,13 @@ fhandler_fifo::update_my_handlers ()
{
debug_printf ("Can't duplicate handle of previous owner, %E");
__seterrno ();
- fc.state = fc_error;
+ fc.set_state (fc_error);
fc.last_read = false;
ret = -1;
}
else
{
- fc.state = shared_fc_handler[i].state;
+ fc.set_state (shared_fc_handler[i].get_state ());
fc.last_read = shared_fc_handler[i].last_read;
}
}
@@ -614,7 +614,7 @@ owner_listen:
break;
default:
debug_printf ("NtFsControlFile status %y after failing to connect bogus client or real client", io.Status);
- fc.state = fc_unknown;
+ fc.set_state (fc_unknown);
break;
}
break;
@@ -1280,7 +1280,7 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
for (j = 0; j < nhandlers; j++)
if (fc_handler[j].last_read)
break;
- if (j < nhandlers && fc_handler[j].state >= fc_closing)
+ if (j < nhandlers && fc_handler[j].get_state () >= fc_closing)
{
NTSTATUS status;
IO_STATUS_BLOCK io;
@@ -1303,11 +1303,11 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
case STATUS_PIPE_EMPTY:
break;
case STATUS_PIPE_BROKEN:
- fc_handler[j].state = fc_disconnected;
+ fc_handler[j].set_state (fc_disconnected);
break;
default:
debug_printf ("NtReadFile status %y", status);
- fc_handler[j].state = fc_error;
+ fc_handler[j].set_state (fc_error);
break;
}
fc_handler[j].last_read = false;
@@ -1315,7 +1315,7 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
/* Second pass. */
for (int i = 0; i < nhandlers; i++)
- if (fc_handler[i].state >= fc_closing)
+ if (fc_handler[i].get_state () >= fc_closing)
{
NTSTATUS status;
IO_STATUS_BLOCK io;
@@ -1339,12 +1339,12 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
case STATUS_PIPE_EMPTY:
break;
case STATUS_PIPE_BROKEN:
- fc_handler[i].state = fc_disconnected;
+ fc_handler[i].set_state (fc_disconnected);
nconnected--;
break;
default:
debug_printf ("NtReadFile status %y", status);
- fc_handler[i].state = fc_error;
+ fc_handler[i].set_state (fc_error);
nconnected--;
break;
}
@@ -1417,45 +1417,51 @@ fhandler_fifo::close_all_handlers ()
fifo_client_unlock ();
}
+/* Return previous state. */
fifo_client_connect_state
-fifo_client_handler::set_state ()
+fifo_client_handler::query_and_set_state ()
{
IO_STATUS_BLOCK io;
FILE_PIPE_LOCAL_INFORMATION fpli;
NTSTATUS status;
+ fifo_client_connect_state prev_state = get_state ();
if (!h)
- return (state = fc_unknown);
+ {
+ set_state (fc_unknown);
+ goto out;
+ }
status = NtQueryInformationFile (h, &io, &fpli,
sizeof (fpli), FilePipeLocalInformation);
if (!NT_SUCCESS (status))
{
debug_printf ("NtQueryInformationFile status %y", status);
- state = fc_error;
+ set_state (fc_error);
}
else if (fpli.ReadDataAvailable > 0)
- state = fc_input_avail;
+ set_state (fc_input_avail);
else
switch (fpli.NamedPipeState)
{
case FILE_PIPE_DISCONNECTED_STATE:
- state = fc_disconnected;
+ set_state (fc_disconnected);
break;
case FILE_PIPE_LISTENING_STATE:
- state = fc_listening;
+ set_state (fc_listening);
break;
case FILE_PIPE_CONNECTED_STATE:
- state = fc_connected;
+ set_state (fc_connected);
break;
case FILE_PIPE_CLOSING_STATE:
- state = fc_closing;
+ set_state (fc_closing);
break;
default:
- state = fc_error;
+ set_state (fc_error);
break;
}
- return state;
+out:
+ return prev_state;
}
void
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 1ba76c817..0c94f6c45 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -875,18 +875,22 @@ peek_fifo (select_record *s, bool from_select)
fh->fifo_client_lock ();
int nconnected = 0;
for (int i = 0; i < fh->get_nhandlers (); i++)
- if (fh->get_fc_handler (i).set_state () >= fc_closing)
- {
- nconnected++;
- if (fh->get_fc_handler (i).state == fc_input_avail)
- {
- select_printf ("read: %s, ready for read", fh->get_name ());
- fh->fifo_client_unlock ();
- fh->reading_unlock ();
- gotone += s->read_ready = true;
- goto out;
- }
- }
+ {
+ fifo_client_handler &fc = fh->get_fc_handler (i);
+ fc.query_and_set_state ();
+ if (fc.get_state () >= fc_closing)
+ {
+ nconnected++;
+ if (fc.get_state () == fc_input_avail)
+ {
+ select_printf ("read: %s, ready for read", fh->get_name ());
+ fh->fifo_client_unlock ();
+ fh->reading_unlock ();
+ gotone += s->read_ready = true;
+ goto out;
+ }
+ }
+ }
fh->fifo_client_unlock ();
if (!nconnected && fh->hit_eof ())
{
--
2.28.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 5/8] Cygwin: FIFO: don't read from pipes that are closing
2020-08-04 12:54 [PATCH 0/8] FIFO: bug fixes and small improvements Ken Brown
` (3 preceding siblings ...)
2020-08-04 12:55 ` [PATCH 4/8] Cygwin: FIFO: reorganize some fifo_client_handler methods Ken Brown
@ 2020-08-04 12:55 ` Ken Brown
2020-08-04 12:55 ` [PATCH 6/8] Cygwin: FIFO: synchronize the fifo_reader and fifosel threads Ken Brown
` (3 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Ken Brown @ 2020-08-04 12:55 UTC (permalink / raw)
To: cygwin-patches
Don't try to read from fifo_client_handlers that are in the fc_closing
state. Experiments have shown that this always yields
STATUS_PIPE_BROKEN, so it just wastes a Windows system call.
Re-order the values in enum fifo_client_connect_state to reflect the
new status of fc_closing.
---
winsup/cygwin/fhandler.h | 9 +--------
winsup/cygwin/fhandler_fifo.cc | 6 +++---
winsup/cygwin/select.cc | 2 +-
3 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index f64eabda4..40e201b0f 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1276,20 +1276,13 @@ public:
#define CYGWIN_FIFO_PIPE_NAME_LEN 47
-/* We view the fc_closing state as borderline between open and closed
- for a writer at the other end of a fifo_client_handler.
-
- We still attempt to read from the writer when the handler is in
- this state, and we don't declare a reader to be at EOF if there's a
- handler in this state. But the existence of a handler in this
- state is not sufficient to unblock a reader trying to open. */
enum fifo_client_connect_state
{
fc_unknown,
fc_error,
fc_disconnected,
- fc_listening,
fc_closing,
+ fc_listening,
fc_connected,
fc_input_avail,
};
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index c816c692a..1e1140f53 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -365,7 +365,7 @@ fhandler_fifo::cleanup_handlers ()
while (i < nhandlers)
{
- if (fc_handler[i].get_state () < fc_closing)
+ if (fc_handler[i].get_state () < fc_connected)
delete_client_handler (i);
else
i++;
@@ -1280,7 +1280,7 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
for (j = 0; j < nhandlers; j++)
if (fc_handler[j].last_read)
break;
- if (j < nhandlers && fc_handler[j].get_state () >= fc_closing)
+ if (j < nhandlers && fc_handler[j].get_state () >= fc_connected)
{
NTSTATUS status;
IO_STATUS_BLOCK io;
@@ -1315,7 +1315,7 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
/* Second pass. */
for (int i = 0; i < nhandlers; i++)
- if (fc_handler[i].get_state () >= fc_closing)
+ if (fc_handler[i].get_state () >= fc_connected)
{
NTSTATUS status;
IO_STATUS_BLOCK io;
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 0c94f6c45..9ee305f64 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -878,7 +878,7 @@ peek_fifo (select_record *s, bool from_select)
{
fifo_client_handler &fc = fh->get_fc_handler (i);
fc.query_and_set_state ();
- if (fc.get_state () >= fc_closing)
+ if (fc.get_state () >= fc_connected)
{
nconnected++;
if (fc.get_state () == fc_input_avail)
--
2.28.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 6/8] Cygwin: FIFO: synchronize the fifo_reader and fifosel threads
2020-08-04 12:54 [PATCH 0/8] FIFO: bug fixes and small improvements Ken Brown
` (4 preceding siblings ...)
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 ` Ken Brown
2020-08-04 12:55 ` [PATCH 7/8] Cygwin: FIFO: fix indentation Ken Brown
` (2 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Ken Brown @ 2020-08-04 12:55 UTC (permalink / raw)
To: cygwin-patches
The fifo_reader thread function and the function select.cc:peek_fifo()
can both change the state of a fifo_client_handler. These changes are
made under fifo_client_lock, so there is no race, but the changes can
still be incompatible.
Add code to make sure that only one of these functions can change the
state from its initial fc_listening state. Whichever function does
this calls the fhandler_fifo::record_connection method, which is now
public so that peek_fifo can call it.
Slightly modify that method to make it suitable for being called by
peek_fifo.
Make a few other small changes to the fifo_reader thread function to
change how it deals with the STATUS_PIPE_CLOSING value that can
(rarely) be returned by NtFsControlFile.
Add commentary to fhandler_fifo.cc to explain fifo_client connect
states and where they can be changed.
---
winsup/cygwin/fhandler.h | 4 +--
winsup/cygwin/fhandler_fifo.cc | 60 ++++++++++++++++++++++++++++++----
winsup/cygwin/select.cc | 5 ++-
3 files changed, 60 insertions(+), 9 deletions(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 40e201b0f..a577ca542 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1413,8 +1413,6 @@ class fhandler_fifo: public fhandler_base
void cleanup_handlers ();
void close_all_handlers ();
void cancel_reader_thread ();
- void record_connection (fifo_client_handler&,
- fifo_client_connect_state = fc_connected);
int create_shmem (bool only_open = false);
int reopen_shmem ();
@@ -1482,6 +1480,8 @@ public:
DWORD fifo_reader_thread_func ();
void fifo_client_lock () { _fifo_client_lock.lock (); }
void fifo_client_unlock () { _fifo_client_lock.unlock (); }
+ void record_connection (fifo_client_handler&, bool = true,
+ fifo_client_connect_state = fc_connected);
int take_ownership (DWORD timeout = INFINITE);
void reading_lock () { shmem->reading_lock (); }
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 1e1140f53..2b829eb6c 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -37,11 +37,42 @@
"fifo_client_handler" structures, one for each writer. A
fifo_client_handler contains the handle for the pipe server
instance and information about the state of the connection with
- the writer. Each writer holds the pipe instance's client handle.
+ the writer. Access to the list is controlled by a
+ "fifo_client_lock".
The reader runs a "fifo_reader_thread" that creates new pipe
instances as needed and listens for client connections.
+ The connection state of a fifo_client_handler has one of the
+ following values, in which order is important:
+
+ fc_unknown
+ fc_error
+ fc_disconnected
+ fc_closing
+ fc_listening
+ fc_connected
+ fc_input_avail
+
+ It can be changed in the following places:
+
+ - It is set to fc_listening when the pipe instance is created.
+
+ - It is set to fc_connected when the fifo_reader_thread detects
+ a connection.
+
+ - It is set to a value reported by the O/S when
+ query_and_set_state is called. This can happen in
+ select.cc:peek_fifo and a couple other places.
+
+ - It is set to fc_disconnected by raw_read when an attempt to
+ read yields STATUS_PIPE_BROKEN.
+
+ - It is set to fc_error in various places when unexpected
+ things happen.
+
+ State changes are always guarded by fifo_client_lock.
+
If there are multiple readers open, only one of them, called the
"owner", maintains the fifo_client_handler list. The owner is
therefore the only reader that can read at any given time. If a
@@ -374,10 +405,11 @@ fhandler_fifo::cleanup_handlers ()
/* Always called with fifo_client_lock in place. */
void
-fhandler_fifo::record_connection (fifo_client_handler& fc,
+fhandler_fifo::record_connection (fifo_client_handler& fc, bool set,
fifo_client_connect_state s)
{
- fc.set_state (s);
+ if (set)
+ fc.set_state (s);
set_pipe_non_blocking (fc.h, true);
}
@@ -583,6 +615,11 @@ owner_listen:
NTSTATUS status1;
fifo_client_lock ();
+ if (fc.get_state () != fc_listening)
+ /* select.cc:peek_fifo has already recorded a connection. */
+ ;
+ else
+ {
switch (status)
{
case STATUS_SUCCESS:
@@ -590,7 +627,12 @@ owner_listen:
record_connection (fc);
break;
case STATUS_PIPE_CLOSING:
- record_connection (fc, fc_closing);
+ debug_printf ("NtFsControlFile got STATUS_PIPE_CLOSING...");
+ /* Maybe a writer already connected, wrote, and closed.
+ Just query the O/S. */
+ fc.query_and_set_state ();
+ debug_printf ("...O/S reports state %d", fc.get_state ());
+ record_connection (fc, false);
break;
case STATUS_THREAD_IS_TERMINATING:
case STATUS_WAIT_1:
@@ -610,17 +652,23 @@ owner_listen:
record_connection (fc);
break;
case STATUS_PIPE_CLOSING:
- record_connection (fc, fc_closing);
+ debug_printf ("got STATUS_PIPE_CLOSING when trying to connect bogus client...");
+ fc.query_and_set_state ();
+ debug_printf ("...O/S reports state %d", fc.get_state ());
+ record_connection (fc, false);
break;
default:
debug_printf ("NtFsControlFile status %y after failing to connect bogus client or real client", io.Status);
- fc.set_state (fc_unknown);
+ fc.set_state (fc_error);
break;
}
break;
default:
+ debug_printf ("NtFsControlFile got unexpected status %y", status);
+ fc.set_state (fc_error);
break;
}
+ }
if (ph)
NtClose (ph);
if (update)
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 9ee305f64..43f07af43 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -877,10 +877,13 @@ peek_fifo (select_record *s, bool from_select)
for (int i = 0; i < fh->get_nhandlers (); i++)
{
fifo_client_handler &fc = fh->get_fc_handler (i);
- fc.query_and_set_state ();
+ fifo_client_connect_state prev_state = fc.query_and_set_state ();
if (fc.get_state () >= fc_connected)
{
nconnected++;
+ if (prev_state == fc_listening)
+ /* The connection was not recorded by the fifo_reader_thread. */
+ fh->record_connection (fc, false);
if (fc.get_state () == fc_input_avail)
{
select_printf ("read: %s, ready for read", fh->get_name ());
--
2.28.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 7/8] Cygwin: FIFO: fix indentation
2020-08-04 12:54 [PATCH 0/8] FIFO: bug fixes and small improvements Ken Brown
` (5 preceding siblings ...)
2020-08-04 12:55 ` [PATCH 6/8] Cygwin: FIFO: synchronize the fifo_reader and fifosel threads Ken Brown
@ 2020-08-04 12:55 ` 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
8 siblings, 0 replies; 10+ messages in thread
From: Ken Brown @ 2020-08-04 12:55 UTC (permalink / raw)
To: cygwin-patches
---
winsup/cygwin/fhandler_fifo.cc | 96 +++++++++++++++++-----------------
1 file changed, 48 insertions(+), 48 deletions(-)
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 2b829eb6c..017d44e54 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -619,56 +619,56 @@ owner_listen:
/* select.cc:peek_fifo has already recorded a connection. */
;
else
- {
- switch (status)
{
- case STATUS_SUCCESS:
- case STATUS_PIPE_CONNECTED:
- record_connection (fc);
- break;
- case STATUS_PIPE_CLOSING:
- debug_printf ("NtFsControlFile got STATUS_PIPE_CLOSING...");
- /* Maybe a writer already connected, wrote, and closed.
- Just query the O/S. */
- fc.query_and_set_state ();
- debug_printf ("...O/S reports state %d", fc.get_state ());
- record_connection (fc, false);
- break;
- case STATUS_THREAD_IS_TERMINATING:
- case STATUS_WAIT_1:
- /* Try to connect a bogus client. Otherwise fc is still
- listening, and the next connection might not get recorded. */
- status1 = open_pipe (ph);
- WaitForSingleObject (conn_evt, INFINITE);
- if (NT_SUCCESS (status1))
- /* Bogus cilent connected. */
- delete_client_handler (nhandlers - 1);
- else
- /* Did a real client connect? */
- switch (io.Status)
- {
- case STATUS_SUCCESS:
- case STATUS_PIPE_CONNECTED:
- record_connection (fc);
- break;
- case STATUS_PIPE_CLOSING:
- debug_printf ("got STATUS_PIPE_CLOSING when trying to connect bogus client...");
- fc.query_and_set_state ();
- debug_printf ("...O/S reports state %d", fc.get_state ());
- record_connection (fc, false);
- break;
- default:
- debug_printf ("NtFsControlFile status %y after failing to connect bogus client or real client", io.Status);
- fc.set_state (fc_error);
- break;
- }
- break;
- default:
- debug_printf ("NtFsControlFile got unexpected status %y", status);
- fc.set_state (fc_error);
- break;
+ switch (status)
+ {
+ case STATUS_SUCCESS:
+ case STATUS_PIPE_CONNECTED:
+ record_connection (fc);
+ break;
+ case STATUS_PIPE_CLOSING:
+ debug_printf ("NtFsControlFile got STATUS_PIPE_CLOSING...");
+ /* Maybe a writer already connected, wrote, and closed.
+ Just query the O/S. */
+ fc.query_and_set_state ();
+ debug_printf ("...O/S reports state %d", fc.get_state ());
+ record_connection (fc, false);
+ break;
+ case STATUS_THREAD_IS_TERMINATING:
+ case STATUS_WAIT_1:
+ /* Try to connect a bogus client. Otherwise fc is still
+ listening, and the next connection might not get recorded. */
+ status1 = open_pipe (ph);
+ WaitForSingleObject (conn_evt, INFINITE);
+ if (NT_SUCCESS (status1))
+ /* Bogus cilent connected. */
+ delete_client_handler (nhandlers - 1);
+ else
+ /* Did a real client connect? */
+ switch (io.Status)
+ {
+ case STATUS_SUCCESS:
+ case STATUS_PIPE_CONNECTED:
+ record_connection (fc);
+ break;
+ case STATUS_PIPE_CLOSING:
+ debug_printf ("got STATUS_PIPE_CLOSING when trying to connect bogus client...");
+ fc.query_and_set_state ();
+ debug_printf ("...O/S reports state %d", fc.get_state ());
+ record_connection (fc, false);
+ break;
+ default:
+ debug_printf ("NtFsControlFile status %y after failing to connect bogus client or real client", io.Status);
+ fc.set_state (fc_error);
+ break;
+ }
+ break;
+ default:
+ debug_printf ("NtFsControlFile got unexpected status %y", status);
+ fc.set_state (fc_error);
+ break;
+ }
}
- }
if (ph)
NtClose (ph);
if (update)
--
2.28.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 8/8] Cygwin: FIFO: add a third pass to raw_read
2020-08-04 12:54 [PATCH 0/8] FIFO: bug fixes and small improvements Ken Brown
` (6 preceding siblings ...)
2020-08-04 12:55 ` [PATCH 7/8] Cygwin: FIFO: fix indentation Ken Brown
@ 2020-08-04 12:55 ` Ken Brown
2020-08-04 15:04 ` [PATCH 0/8] FIFO: bug fixes and small improvements Corinna Vinschen
8 siblings, 0 replies; 10+ messages in thread
From: Ken Brown @ 2020-08-04 12:55 UTC (permalink / raw)
To: cygwin-patches
Currently raw_read makes two passes through the list of clients. On
the first pass it tries to read from the client from which it last
read successfully. On the second pass it tries to read from all
connected clients.
Add a new pass in between these two, in which raw_read tries to read
from all clients that are in the fc_input_avail case. This should be
more efficient in case select was previously called and detected input
available.
Slightly tweak the first pass. If a client is marked as having the
last successful read but reading from it now finds no input, don't
unmark it unless we successfully read from a different client on one
of the later passes.
---
winsup/cygwin/fhandler_fifo.cc | 66 ++++++++++++++++++++++++++++++----
1 file changed, 60 insertions(+), 6 deletions(-)
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 017d44e54..a33c32b73 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -1318,17 +1318,30 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
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
- we last read successfully. This should minimize
- interleaving of writes from different clients. */
fifo_client_lock ();
+ /* Poll the connected clients for input. Make three passes.
+
+ On the first pass, just try to read from the client from
+ which we last read successfully. This should minimize
+ interleaving of writes from different clients.
+
+ On the second pass, just try to read from the clients in the
+ state fc_input_avail. This should be more efficient if
+ select has been called and detected input available.
+
+ On the third pass, try to read from all connected clients. */
+
/* First pass. */
int j;
for (j = 0; j < nhandlers; j++)
if (fc_handler[j].last_read)
break;
- if (j < nhandlers && fc_handler[j].get_state () >= fc_connected)
+ if (j < nhandlers && fc_handler[j].get_state () < fc_connected)
+ {
+ fc_handler[j].last_read = false;
+ j = nhandlers;
+ }
+ if (j < nhandlers)
{
NTSTATUS status;
IO_STATUS_BLOCK io;
@@ -1349,6 +1362,8 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
}
break;
case STATUS_PIPE_EMPTY:
+ /* Update state in case it's fc_input_avail. */
+ fc_handler[j].set_state (fc_connected);
break;
case STATUS_PIPE_BROKEN:
fc_handler[j].set_state (fc_disconnected);
@@ -1358,10 +1373,47 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
fc_handler[j].set_state (fc_error);
break;
}
- fc_handler[j].last_read = false;
}
/* Second pass. */
+ for (int i = 0; i < nhandlers; i++)
+ if (fc_handler[i].get_state () == fc_input_avail)
+ {
+ NTSTATUS status;
+ IO_STATUS_BLOCK io;
+
+ status = NtReadFile (fc_handler[i].h, NULL, NULL, NULL,
+ &io, in_ptr, len, NULL, NULL);
+ switch (status)
+ {
+ case STATUS_SUCCESS:
+ case STATUS_BUFFER_OVERFLOW:
+ if (io.Information > 0)
+ {
+ len = io.Information;
+ if (j < nhandlers)
+ fc_handler[j].last_read = false;
+ fc_handler[i].last_read = true;
+ fifo_client_unlock ();
+ reading_unlock ();
+ return;
+ }
+ break;
+ case STATUS_PIPE_EMPTY:
+ /* No input available after all. */
+ fc_handler[i].set_state (fc_connected);
+ break;
+ case STATUS_PIPE_BROKEN:
+ fc_handler[i].set_state (fc_disconnected);
+ break;
+ default:
+ debug_printf ("NtReadFile status %y", status);
+ fc_handler[i].set_state (fc_error);
+ break;
+ }
+ }
+
+ /* Third pass. */
for (int i = 0; i < nhandlers; i++)
if (fc_handler[i].get_state () >= fc_connected)
{
@@ -1378,6 +1430,8 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
if (io.Information > 0)
{
len = io.Information;
+ if (j < nhandlers)
+ fc_handler[j].last_read = false;
fc_handler[i].last_read = true;
fifo_client_unlock ();
reading_unlock ();
--
2.28.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/8] FIFO: bug fixes and small improvements
2020-08-04 12:54 [PATCH 0/8] FIFO: bug fixes and small improvements Ken Brown
` (7 preceding siblings ...)
2020-08-04 12:55 ` [PATCH 8/8] Cygwin: FIFO: add a third pass to raw_read Ken Brown
@ 2020-08-04 15:04 ` Corinna Vinschen
8 siblings, 0 replies; 10+ messages in thread
From: Corinna Vinschen @ 2020-08-04 15:04 UTC (permalink / raw)
To: cygwin-patches
On Aug 4 08:54, Ken Brown via Cygwin-patches wrote:
> The second patch in this series fixes a serious bug that has resulted
> in observable hangs. The other patches are minor bug fixes or minor
> performance improvements.
>
> Ken Brown (8):
> Cygwin: FIFO: lock fixes
> Cygwin: FIFO: fix timing issue with owner change
> Cygwin: FIFO: add a timeout to take_ownership
> Cygwin: FIFO: reorganize some fifo_client_handler methods
> Cygwin: FIFO: don't read from pipes that are closing
> Cygwin: FIFO: synchronize the fifo_reader and fifosel threads
> Cygwin: FIFO: fix indentation
> Cygwin: FIFO: add a third pass to raw_read
>
> winsup/cygwin/fhandler.h | 24 +--
> winsup/cygwin/fhandler_fifo.cc | 358 ++++++++++++++++++++++-----------
> winsup/cygwin/select.cc | 38 ++--
> 3 files changed, 268 insertions(+), 152 deletions(-)
>
> --
> 2.28.0
LGTM
Corinna
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-08-04 15:04 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 ` [PATCH 3/8] Cygwin: FIFO: add a timeout to take_ownership Ken Brown
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
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).