* [PATCH 00/12] FIFO: fix multiple reader support
@ 2020-07-16 16:19 Ken Brown
2020-07-16 16:19 ` [PATCH 01/12] Cygwin: FIFO: fix problems finding new owner Ken Brown
` (12 more replies)
0 siblings, 13 replies; 14+ messages in thread
From: Ken Brown @ 2020-07-16 16:19 UTC (permalink / raw)
To: cygwin-patches
There were several flaws in my previous attempt to add support for
explicitly opening a FIFO multiple times for reading. (By
"explicitly" I mean by calling open rather than by calling
fork/exec/dup.) See
https://sourceware.org/pipermail/cygwin/2020-July/245456.html
for one indication of problems
The most important flaw was that I tried to use an indirect,
unreliable method for determining whether there are writers open.
This is fixed in the second patch of this series by adding a member
'_nwriters' to struct fifo_shmem_t, which counts the number of open
writers.
We now have to give writers access to the shared memory as well as
readers, so that they can increment _nwriters in open/fork/exec/dup
and decrement it in close.
The other patches contain miscellaneous fixes/improvements.
Ken Brown (12):
Cygwin: FIFO: fix problems finding new owner
Cygwin: FIFO: keep a writer count in shared memory
Cygwin: fhandler_fifo::hit_eof: improve reliability
Cygwin: FIFO: reduce I/O interleaving
Cygwin: FIFO: improve taking ownership in fifo_reader_thread
Cygwin: FIFO: fix indentation
Cygwin: FIFO: make certain errors non-fatal
Cygwin: FIFO: add missing lock
Cygwin: fhandler_fifo::take_ownership: don't set event unnecessarily
Cygwin: FIFO: allow take_ownership to be interrupted
Cygwin: FIFO: clean up
Cygwin: FIFO: update commentary
winsup/cygwin/fhandler.h | 55 +--
winsup/cygwin/fhandler_fifo.cc | 725 ++++++++++++++++++---------------
winsup/cygwin/select.cc | 14 +-
3 files changed, 433 insertions(+), 361 deletions(-)
--
2.27.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 01/12] Cygwin: FIFO: fix problems finding new owner
2020-07-16 16:19 [PATCH 00/12] FIFO: fix multiple reader support Ken Brown
@ 2020-07-16 16:19 ` Ken Brown
2020-07-16 16:19 ` [PATCH 02/12] Cygwin: FIFO: keep a writer count in shared memory Ken Brown
` (11 subsequent siblings)
12 siblings, 0 replies; 14+ messages in thread
From: Ken Brown @ 2020-07-16 16:19 UTC (permalink / raw)
To: cygwin-patches
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 | 8 +++-
winsup/cygwin/fhandler_fifo.cc | 86 +++++++++++++++++++++-------------
2 files changed, 61 insertions(+), 33 deletions(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 7a28adc16..cf6daea06 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1328,7 +1328,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. */
@@ -1336,6 +1336,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); }
@@ -1352,6 +1353,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); }
@@ -1420,8 +1423,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)
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 3d34cdfab..2d4f7a97e 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/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. */
- bool find_new_owner = false;
+ /* If we're the owner, we can't close our fc_handlers if a new
+ owner might need to duplicate them. */
+ bool close_fc_ok = false;
cancel_reader_thread ();
- owner_lock ();
- if (get_owner () == me)
+ nreaders_lock ();
+ if (dec_nreaders () == 0)
{
- find_new_owner = true;
+ close_fc_ok = true;
+ ResetEvent (read_ready);
+ ResetEvent (owner_needed_evt);
+ ResetEvent (owner_found_evt);
set_owner (null_fr_id);
- set_prev_owner (me);
- owner_needed ();
+ set_prev_owner (null_fr_id);
+ set_pending_owner (null_fr_id);
+ set_shared_nhandlers (0);
}
- owner_unlock ();
- if (dec_nreaders () == 0)
- ResetEvent (read_ready);
- else if (find_new_owner && !IsEventSignalled (owner_found_evt))
+ else
{
- bool found = false;
- do
- if (dec_nreaders () >= 0)
- {
- /* There's still another reader open. */
- if (WaitForSingleObject (owner_found_evt, 1) == WAIT_OBJECT_0)
- found = true;
- else
- {
- owner_lock ();
- if (get_owner ()) /* We missed owner_found_evt? */
- found = true;
- else
- owner_needed ();
- owner_unlock ();
- }
- }
- while (inc_nreaders () > 0 && !found);
+ owner_lock ();
+ if (get_owner () != me)
+ close_fc_ok = true;
+ else
+ {
+ set_owner (null_fr_id);
+ 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)
--
2.27.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 02/12] Cygwin: FIFO: keep a writer count in shared memory
2020-07-16 16:19 [PATCH 00/12] FIFO: fix multiple reader support Ken Brown
2020-07-16 16:19 ` [PATCH 01/12] Cygwin: FIFO: fix problems finding new owner Ken Brown
@ 2020-07-16 16:19 ` Ken Brown
2020-07-16 16:19 ` [PATCH 03/12] Cygwin: fhandler_fifo::hit_eof: improve reliability Ken Brown
` (10 subsequent siblings)
12 siblings, 0 replies; 14+ messages in thread
From: Ken Brown @ 2020-07-16 16:19 UTC (permalink / raw)
To: cygwin-patches
When a reader opens, it needs to block if there are no writers open
(unless is is opened with O_NONBLOCK). This is easy for the first
reader to test, since it can just wait for a writer to signal that it
is open (via the write_ready event). But when a second reader wants
to open, all writers might have closed.
To check this, use a new '_nwriters' member of struct fifo_shmem_t,
which keeps track of the number of open writers. This should be more
reliable than the previous method.
Add nwriters_lock to control access to shmem->_nwriters, and remove
reader_opening_lock, which is no longer needed.
Previously only readers had access to the shared memory, but now
writers access it too so that they can increment _nwriters during
open/dup/fork/exec and decrement it during close.
Add an optional 'only_open' argument to create_shmem for use by
writers, which only open the shared memory rather than first trying to
create it. Since writers don't need to access the shared memory until
they have successfully connected to a pipe instance, they can safely
assume that a reader has already created the shared memory.
For debugging purposes, change create_shmem to return 1 instead of 0
when a reader successfully opens the shared memory after finding that
it had already been created.
Remove check_write_ready_evt, write_ready_ok_evt, and
check_write_ready(), which are no longer needed.
When opening a writer and looping to try to get a connection, recheck
read_ready at the top of the loop since the number of readers might
have changed.
To slightly speed up the process of opening the first reader, take
ownership immediately rather than waiting for the fifo_reader_thread
to handle it.
---
winsup/cygwin/fhandler.h | 27 ++--
winsup/cygwin/fhandler_fifo.cc | 263 ++++++++++++++-------------------
2 files changed, 124 insertions(+), 166 deletions(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index cf6daea06..f034a110e 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1323,12 +1323,14 @@ struct fifo_reader_id_t
}
};
-/* Info needed by all readers of a FIFO, stored in named shared memory. */
+/* Info needed by all fhandlers for a given FIFO, stored in named
+ shared memory. This is mostly for readers, but writers need access
+ in order to update the count of open writers. */
class fifo_shmem_t
{
- LONG _nreaders;
+ LONG _nreaders, _nwriters;
fifo_reader_id_t _owner, _prev_owner, _pending_owner;
- af_unix_spinlock_t _owner_lock, _reading_lock, _reader_opening_lock, _nreaders_lock;
+ af_unix_spinlock_t _owner_lock, _reading_lock, _nreaders_lock, _nwriters_lock;
/* Info about shared memory block used for temporary storage of the
owner's fc_handler list. */
@@ -1339,6 +1341,9 @@ public:
int nreaders () const { return (int) _nreaders; }
int inc_nreaders () { return (int) InterlockedIncrement (&_nreaders); }
int dec_nreaders () { return (int) InterlockedDecrement (&_nreaders); }
+ int nwriters () const { return (int) _nwriters; }
+ int inc_nwriters () { return (int) InterlockedIncrement (&_nwriters); }
+ int dec_nwriters () { return (int) InterlockedDecrement (&_nwriters); }
fifo_reader_id_t get_owner () const { return _owner; }
void set_owner (fifo_reader_id_t fr_id) { _owner = fr_id; }
@@ -1351,10 +1356,10 @@ public:
void owner_unlock () { _owner_lock.unlock (); }
void reading_lock () { _reading_lock.lock (); }
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 (); }
+ void nwriters_lock () { _nwriters_lock.lock (); }
+ void nwriters_unlock () { _nwriters_lock.unlock (); }
int get_shared_nhandlers () const { return (int) _sh_nhandlers; }
void set_shared_nhandlers (int n) { InterlockedExchange (&_sh_nhandlers, n); }
@@ -1380,8 +1385,6 @@ class fhandler_fifo: public fhandler_base
HANDLE owner_needed_evt; /* The owner is closing. */
HANDLE owner_found_evt; /* A new owner has taken over. */
HANDLE update_needed_evt; /* shared_fc_handler needs updating. */
- HANDLE check_write_ready_evt; /* write_ready needs to be checked. */
- HANDLE write_ready_ok_evt; /* check_write_ready is done. */
/* Handles to non-shared events needed for fifo_reader_threads. */
HANDLE cancel_evt; /* Signal thread to terminate. */
@@ -1417,7 +1420,7 @@ class fhandler_fifo: public fhandler_base
void record_connection (fifo_client_handler&,
fifo_client_connect_state = fc_connected);
- int create_shmem ();
+ int create_shmem (bool only_open = false);
int reopen_shmem ();
int create_shared_fc_handler ();
int reopen_shared_fc_handler ();
@@ -1426,8 +1429,13 @@ class fhandler_fifo: public fhandler_base
int nreaders () const { return shmem->nreaders (); }
int inc_nreaders () { return shmem->inc_nreaders (); }
int dec_nreaders () { return shmem->dec_nreaders (); }
+ int nwriters () const { return shmem->nwriters (); }
+ int inc_nwriters () { return shmem->inc_nwriters (); }
+ int dec_nwriters () { return shmem->dec_nwriters (); }
void nreaders_lock () { shmem->nreaders_lock (); }
void nreaders_unlock () { shmem->nreaders_unlock (); }
+ void nwriters_lock () { shmem->nwriters_lock (); }
+ void nwriters_unlock () { shmem->nwriters_unlock (); }
fifo_reader_id_t get_prev_owner () const { return shmem->get_prev_owner (); }
void set_prev_owner (fifo_reader_id_t fr_id)
@@ -1462,9 +1470,6 @@ class fhandler_fifo: public fhandler_base
{ return shmem->shared_fc_handler_updated (); }
void shared_fc_handler_updated (bool val)
{ shmem->shared_fc_handler_updated (val); }
- void check_write_ready ();
- void reader_opening_lock () { shmem->reader_opening_lock (); }
- void reader_opening_unlock () { shmem->reader_opening_unlock (); }
public:
fhandler_fifo ();
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 2d4f7a97e..26b24d019 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -87,7 +87,6 @@ fhandler_fifo::fhandler_fifo ():
fhandler_base (),
read_ready (NULL), write_ready (NULL), writer_opening (NULL),
owner_needed_evt (NULL), owner_found_evt (NULL), update_needed_evt (NULL),
- check_write_ready_evt (NULL), write_ready_ok_evt (NULL),
cancel_evt (NULL), thr_sync_evt (NULL), _maybe_eof (false),
fc_handler (NULL), shandlers (0), nhandlers (0),
reader (false), writer (false), duplexer (false),
@@ -429,24 +428,6 @@ fhandler_fifo::update_shared_handlers ()
return 0;
}
-/* The write_ready event gets set when a writer opens, to indicate
- that a blocking reader can open. If a second reader wants to open,
- we need to see if there are still any writers open. */
-void
-fhandler_fifo::check_write_ready ()
-{
- bool set = false;
-
- for (int i = 0; i < nhandlers && !set; i++)
- if (fc_handler[i].set_state () >= fc_connected)
- set = true;
- if (set || IsEventSignalled (writer_opening))
- SetEvent (write_ready);
- else
- ResetEvent (write_ready);
- SetEvent (write_ready_ok_evt);
-}
-
static DWORD WINAPI
fifo_reader_thread (LPVOID param)
{
@@ -532,15 +513,13 @@ fhandler_fifo::fifo_reader_thread_func ()
IO_STATUS_BLOCK io;
bool cancel = false;
bool update = false;
- bool check = false;
status = NtFsControlFile (fc.h, conn_evt, NULL, NULL, &io,
FSCTL_PIPE_LISTEN, NULL, 0, NULL, 0);
if (status == STATUS_PENDING)
{
- HANDLE w[4] = { conn_evt, update_needed_evt,
- check_write_ready_evt, cancel_evt };
- switch (WaitForMultipleObjects (4, w, false, INFINITE))
+ HANDLE w[3] = { conn_evt, update_needed_evt, cancel_evt };
+ switch (WaitForMultipleObjects (3, w, false, INFINITE))
{
case WAIT_OBJECT_0:
status = io.Status;
@@ -552,10 +531,6 @@ fhandler_fifo::fifo_reader_thread_func ()
update = true;
break;
case WAIT_OBJECT_0 + 2:
- status = STATUS_WAIT_2;
- check = true;
- break;
- case WAIT_OBJECT_0 + 3:
status = STATUS_THREAD_IS_TERMINATING;
cancel = true;
update = true;
@@ -582,7 +557,6 @@ fhandler_fifo::fifo_reader_thread_func ()
break;
case STATUS_THREAD_IS_TERMINATING:
case STATUS_WAIT_1:
- case STATUS_WAIT_2:
/* Try to connect a bogus client. Otherwise fc is still
listening, and the next connection might not get recorded. */
status1 = open_pipe (ph);
@@ -614,8 +588,6 @@ fhandler_fifo::fifo_reader_thread_func ()
NtClose (ph);
if (update && update_shared_handlers () < 0)
api_fatal ("Can't update shared handlers, %E");
- if (check)
- check_write_ready ();
fifo_client_unlock ();
if (cancel)
goto canceled;
@@ -629,8 +601,14 @@ canceled:
return 0;
}
+/* Return -1 on error and 0 or 1 on success. If ONLY_OPEN is true, we
+ expect the shared memory to exist, and we only try to open it. In
+ this case, we return 0 on success.
+
+ Otherwise, we create the shared memory if it doesn't exist, and we
+ return 1 if it already existed and we successfully open it. */
int
-fhandler_fifo::create_shmem ()
+fhandler_fifo::create_shmem (bool only_open)
{
HANDLE sect;
OBJECT_ATTRIBUTES attr;
@@ -640,16 +618,22 @@ fhandler_fifo::create_shmem ()
PVOID addr = NULL;
UNICODE_STRING uname;
WCHAR shmem_name[MAX_PATH];
+ bool already_exists = false;
__small_swprintf (shmem_name, L"fifo-shmem.%08x.%016X", get_dev (),
get_ino ());
RtlInitUnicodeString (&uname, shmem_name);
InitializeObjectAttributes (&attr, &uname, OBJ_INHERIT,
get_shared_parent_dir (), NULL);
- status = NtCreateSection (§, STANDARD_RIGHTS_REQUIRED | SECTION_QUERY
- | SECTION_MAP_READ | SECTION_MAP_WRITE,
- &attr, &size, PAGE_READWRITE, SEC_COMMIT, NULL);
- if (status == STATUS_OBJECT_NAME_COLLISION)
+ if (!only_open)
+ {
+ status = NtCreateSection (§, STANDARD_RIGHTS_REQUIRED | SECTION_QUERY
+ | SECTION_MAP_READ | SECTION_MAP_WRITE,
+ &attr, &size, PAGE_READWRITE, SEC_COMMIT, NULL);
+ if (status == STATUS_OBJECT_NAME_COLLISION)
+ already_exists = true;
+ }
+ if (only_open || already_exists)
status = NtOpenSection (§, STANDARD_RIGHTS_REQUIRED | SECTION_QUERY
| SECTION_MAP_READ | SECTION_MAP_WRITE, &attr);
if (!NT_SUCCESS (status))
@@ -667,7 +651,7 @@ fhandler_fifo::create_shmem ()
}
shmem_handle = sect;
shmem = (fifo_shmem_t *) addr;
- return 0;
+ return already_exists ? 1 : 0;
}
/* shmem_handle must be valid when this is called. */
@@ -787,7 +771,7 @@ fhandler_fifo::remap_shared_fc_handler (size_t nbytes)
int
fhandler_fifo::open (int flags, mode_t)
{
- int saved_errno = 0;
+ int saved_errno = 0, shmem_res = 0;
if (flags & O_PATH)
return open_fs (flags);
@@ -802,8 +786,7 @@ fhandler_fifo::open (int flags, mode_t)
writer = true;
break;
case O_RDWR:
- reader = true;
- duplexer = true;
+ reader = writer = duplexer = true;
break;
default:
set_errno (EINVAL);
@@ -844,29 +827,26 @@ fhandler_fifo::open (int flags, mode_t)
goto err_close_write_ready;
}
- /* If we're reading, signal read_ready, create the shared memory,
- and start the fifo_reader_thread. */
+ /* If we're reading, create the shared memory and the shared
+ fc_handler memory, create some events, start the
+ fifo_reader_thread, signal read_ready, and wait for a writer. */
if (reader)
{
- bool first = true;
-
- SetEvent (read_ready);
- if (create_shmem () < 0)
+ /* Create/open shared memory. */
+ if ((shmem_res = create_shmem ()) < 0)
goto err_close_writer_opening;
+ else if (shmem_res == 0)
+ debug_printf ("shmem created");
+ else
+ debug_printf ("shmem existed; ok if we're not the first reader");
if (create_shared_fc_handler () < 0)
goto err_close_shmem;
- reader_opening_lock ();
- if (inc_nreaders () == 1)
- /* Reinitialize _sh_fc_handler_updated, which starts as 0. */
- shared_fc_handler_updated (true);
- else
- first = false;
npbuf[0] = 'n';
if (!(owner_needed_evt = CreateEvent (sa_buf, true, false, npbuf)))
{
debug_printf ("CreateEvent for %s failed, %E", npbuf);
__seterrno ();
- goto err_dec_nreaders;
+ goto err_close_shared_fc_handler;
}
npbuf[0] = 'f';
if (!(owner_found_evt = CreateEvent (sa_buf, true, false, npbuf)))
@@ -882,36 +862,23 @@ fhandler_fifo::open (int flags, mode_t)
__seterrno ();
goto err_close_owner_found_evt;
}
- npbuf[0] = 'c';
- if (!(check_write_ready_evt = CreateEvent (sa_buf, false, false, npbuf)))
- {
- debug_printf ("CreateEvent for %s failed, %E", npbuf);
- __seterrno ();
- goto err_close_update_needed_evt;
- }
- npbuf[0] = 'k';
- if (!(write_ready_ok_evt = CreateEvent (sa_buf, false, false, npbuf)))
- {
- debug_printf ("CreateEvent for %s failed, %E", npbuf);
- __seterrno ();
- goto err_close_check_write_ready_evt;
- }
if (!(cancel_evt = create_event ()))
- goto err_close_write_ready_ok_evt;
+ goto err_close_update_needed_evt;
if (!(thr_sync_evt = create_event ()))
goto err_close_cancel_evt;
+
me.winpid = GetCurrentProcessId ();
me.fh = this;
- new cygthread (fifo_reader_thread, this, "fifo_reader", thr_sync_evt);
- /* Wait until there's an owner. */
- owner_lock ();
- while (!get_owner ())
+ nreaders_lock ();
+ if (inc_nreaders () == 1)
{
- owner_unlock ();
- yield ();
- owner_lock ();
+ /* Reinitialize _sh_fc_handler_updated, which starts as 0. */
+ shared_fc_handler_updated (true);
+ set_owner (me);
}
- owner_unlock ();
+ new cygthread (fifo_reader_thread, this, "fifo_reader", thr_sync_evt);
+ SetEvent (read_ready);
+ nreaders_unlock ();
/* If we're a duplexer, we need a handle for writing. */
if (duplexer)
@@ -919,6 +886,11 @@ fhandler_fifo::open (int flags, mode_t)
HANDLE ph = NULL;
NTSTATUS status;
+ nwriters_lock ();
+ inc_nwriters ();
+ SetEvent (write_ready);
+ nwriters_unlock ();
+
while (1)
{
status = open_pipe (ph);
@@ -937,46 +909,39 @@ fhandler_fifo::open (int flags, mode_t)
else
{
__seterrno_from_nt_status (status);
+ nohandle (true);
goto err_close_reader;
}
}
}
/* Not a duplexer; wait for a writer to connect if we're blocking. */
- else if (!(flags & O_NONBLOCK))
- {
- if (!first)
- {
- /* Ask the owner to update write_ready. */
- SetEvent (check_write_ready_evt);
- WaitForSingleObject (write_ready_ok_evt, INFINITE);
- }
- if (!wait (write_ready))
- goto err_close_reader;
- }
- reader_opening_unlock ();
+ else if (!wait (write_ready))
+ goto err_close_reader;
goto success;
}
- /* If we're writing, wait for read_ready, connect to the pipe, and
- signal write_ready. */
+ /* If we're writing, wait for read_ready, connect to the pipe, open
+ the shared memory, and signal write_ready. */
if (writer)
{
NTSTATUS status;
+ /* Don't let a reader see EOF at this point. */
SetEvent (writer_opening);
- if (!wait (read_ready))
- {
- ResetEvent (writer_opening);
- goto err_close_writer_opening;
- }
while (1)
{
+ if (!wait (read_ready))
+ {
+ ResetEvent (writer_opening);
+ goto err_close_writer_opening;
+ }
status = open_pipe (get_handle ());
if (NT_SUCCESS (status))
- goto writer_success;
+ goto writer_shmem;
else if (status == STATUS_OBJECT_NAME_NOT_FOUND)
{
- /* The pipe hasn't been created yet. */
+ /* The pipe hasn't been created yet or there's no longer
+ a reader open. */
yield ();
continue;
}
@@ -995,7 +960,6 @@ fhandler_fifo::open (int flags, mode_t)
and/or many writers are trying to connect simultaneously */
while (1)
{
- SetEvent (writer_opening);
if (!wait (read_ready))
{
ResetEvent (writer_opening);
@@ -1003,7 +967,7 @@ fhandler_fifo::open (int flags, mode_t)
}
status = wait_open_pipe (get_handle ());
if (NT_SUCCESS (status))
- goto writer_success;
+ goto writer_shmem;
else if (status == STATUS_IO_TIMEOUT)
continue;
else
@@ -1015,34 +979,34 @@ fhandler_fifo::open (int flags, mode_t)
}
}
}
-writer_success:
+writer_shmem:
+ if (create_shmem (true) < 0)
+ goto err_close_writer_opening;
+/* writer_success: */
set_pipe_non_blocking (get_handle (), flags & O_NONBLOCK);
+ nwriters_lock ();
+ inc_nwriters ();
SetEvent (write_ready);
+ ResetEvent (writer_opening);
+ nwriters_unlock ();
success:
return 1;
err_close_reader:
saved_errno = get_errno ();
close ();
set_errno (saved_errno);
- reader_opening_unlock ();
return 0;
+/* err_close_thr_sync_evt: */
+/* NtClose (thr_sync_evt); */
err_close_cancel_evt:
NtClose (cancel_evt);
-err_close_write_ready_ok_evt:
- NtClose (write_ready_ok_evt);
-err_close_check_write_ready_evt:
- NtClose (check_write_ready_evt);
err_close_update_needed_evt:
NtClose (update_needed_evt);
err_close_owner_found_evt:
NtClose (owner_found_evt);
err_close_owner_needed_evt:
NtClose (owner_needed_evt);
-err_dec_nreaders:
- if (dec_nreaders () == 0)
- ResetEvent (read_ready);
- reader_opening_unlock ();
-/* err_close_shared_fc_handler: */
+err_close_shared_fc_handler:
NtUnmapViewOfSection (NtCurrentProcess (), shared_fc_handler);
NtClose (shared_fc_hdl);
err_close_shmem:
@@ -1416,6 +1380,13 @@ fhandler_fifo::cancel_reader_thread ()
int
fhandler_fifo::close ()
{
+ if (writer)
+ {
+ nwriters_lock ();
+ if (dec_nwriters () == 0)
+ ResetEvent (write_ready);
+ nwriters_unlock ();
+ }
if (reader)
{
/* If we're the owner, we can't close our fc_handlers if a new
@@ -1481,23 +1452,19 @@ fhandler_fifo::close ()
NtClose (owner_found_evt);
if (update_needed_evt)
NtClose (update_needed_evt);
- if (check_write_ready_evt)
- NtClose (check_write_ready_evt);
- if (write_ready_ok_evt)
- NtClose (write_ready_ok_evt);
if (cancel_evt)
NtClose (cancel_evt);
if (thr_sync_evt)
NtClose (thr_sync_evt);
- if (shmem)
- NtUnmapViewOfSection (NtCurrentProcess (), shmem);
- if (shmem_handle)
- NtClose (shmem_handle);
if (shared_fc_handler)
NtUnmapViewOfSection (NtCurrentProcess (), shared_fc_handler);
if (shared_fc_hdl)
NtClose (shared_fc_hdl);
}
+ if (shmem)
+ NtUnmapViewOfSection (NtCurrentProcess (), shmem);
+ if (shmem_handle)
+ NtClose (shmem_handle);
if (read_ready)
NtClose (read_ready);
if (write_ready)
@@ -1560,6 +1527,15 @@ fhandler_fifo::dup (fhandler_base *child, int flags)
__seterrno ();
goto err_close_write_ready;
}
+ if (!DuplicateHandle (GetCurrentProcess (), shmem_handle,
+ GetCurrentProcess (), &fhf->shmem_handle,
+ 0, !(flags & O_CLOEXEC), DUPLICATE_SAME_ACCESS))
+ {
+ __seterrno ();
+ goto err_close_writer_opening;
+ }
+ if (fhf->reopen_shmem () < 0)
+ goto err_close_shmem_handle;
if (reader)
{
/* Make sure the child starts unlocked. */
@@ -1569,15 +1545,6 @@ fhandler_fifo::dup (fhandler_base *child, int flags)
fhf->nhandlers = fhf->shandlers = 0;
fhf->fc_handler = NULL;
- if (!DuplicateHandle (GetCurrentProcess (), shmem_handle,
- GetCurrentProcess (), &fhf->shmem_handle,
- 0, !(flags & O_CLOEXEC), DUPLICATE_SAME_ACCESS))
- {
- __seterrno ();
- goto err_close_writer_opening;
- }
- if (fhf->reopen_shmem () < 0)
- goto err_close_shmem_handle;
if (!DuplicateHandle (GetCurrentProcess (), shared_fc_hdl,
GetCurrentProcess (), &fhf->shared_fc_hdl,
0, !(flags & O_CLOEXEC), DUPLICATE_SAME_ACCESS))
@@ -1608,35 +1575,19 @@ fhandler_fifo::dup (fhandler_base *child, int flags)
__seterrno ();
goto err_close_owner_found_evt;
}
- if (!DuplicateHandle (GetCurrentProcess (), check_write_ready_evt,
- GetCurrentProcess (), &fhf->check_write_ready_evt,
- 0, !(flags & O_CLOEXEC), DUPLICATE_SAME_ACCESS))
- {
- __seterrno ();
- goto err_close_update_needed_evt;
- }
- if (!DuplicateHandle (GetCurrentProcess (), write_ready_ok_evt,
- GetCurrentProcess (), &fhf->write_ready_ok_evt,
- 0, !(flags & O_CLOEXEC), DUPLICATE_SAME_ACCESS))
- {
- __seterrno ();
- goto err_close_check_write_ready_evt;
- }
if (!(fhf->cancel_evt = create_event ()))
- goto err_close_write_ready_ok_evt;
+ goto err_close_update_needed_evt;
if (!(fhf->thr_sync_evt = create_event ()))
goto err_close_cancel_evt;
inc_nreaders ();
fhf->me.fh = fhf;
new cygthread (fifo_reader_thread, fhf, "fifo_reader", fhf->thr_sync_evt);
}
+ if (writer)
+ inc_nwriters ();
return 0;
err_close_cancel_evt:
NtClose (fhf->cancel_evt);
-err_close_write_ready_ok_evt:
- NtClose (fhf->write_ready_ok_evt);
-err_close_check_write_ready_evt:
- NtClose (fhf->check_write_ready_evt);
err_close_update_needed_evt:
NtClose (fhf->update_needed_evt);
err_close_owner_found_evt:
@@ -1668,22 +1619,20 @@ fhandler_fifo::fixup_after_fork (HANDLE parent)
fork_fixup (parent, read_ready, "read_ready");
fork_fixup (parent, write_ready, "write_ready");
fork_fixup (parent, writer_opening, "writer_opening");
+ fork_fixup (parent, shmem_handle, "shmem_handle");
+ if (reopen_shmem () < 0)
+ api_fatal ("Can't reopen shared memory during fork, %E");
if (reader)
{
/* Make sure the child starts unlocked. */
fifo_client_unlock ();
- fork_fixup (parent, shmem_handle, "shmem_handle");
- if (reopen_shmem () < 0)
- api_fatal ("Can't reopen shared memory during fork, %E");
fork_fixup (parent, shared_fc_hdl, "shared_fc_hdl");
if (reopen_shared_fc_handler () < 0)
api_fatal ("Can't reopen shared fc_handler memory during fork, %E");
fork_fixup (parent, owner_needed_evt, "owner_needed_evt");
fork_fixup (parent, owner_found_evt, "owner_found_evt");
fork_fixup (parent, update_needed_evt, "update_needed_evt");
- fork_fixup (parent, check_write_ready_evt, "check_write_ready_evt");
- fork_fixup (parent, write_ready_ok_evt, "write_ready_ok_evt");
if (close_on_exec ())
/* Prevent a later attempt to close the non-inherited
pipe-instance handles copied from the parent. */
@@ -1696,19 +1645,23 @@ fhandler_fifo::fixup_after_fork (HANDLE parent)
me.winpid = GetCurrentProcessId ();
new cygthread (fifo_reader_thread, this, "fifo_reader", thr_sync_evt);
}
+ if (writer)
+ inc_nwriters ();
}
void
fhandler_fifo::fixup_after_exec ()
{
fhandler_base::fixup_after_exec ();
- if (reader && !close_on_exec ())
+ if (close_on_exec ())
+ return;
+ if (reopen_shmem () < 0)
+ api_fatal ("Can't reopen shared memory during exec, %E");
+ if (reader)
{
/* Make sure the child starts unlocked. */
fifo_client_unlock ();
- if (reopen_shmem () < 0)
- api_fatal ("Can't reopen shared memory during exec, %E");
if (reopen_shared_fc_handler () < 0)
api_fatal ("Can't reopen shared fc_handler memory during exec, %E");
fc_handler = NULL;
@@ -1723,6 +1676,8 @@ fhandler_fifo::fixup_after_exec ()
me.winpid = GetCurrentProcessId ();
new cygthread (fifo_reader_thread, this, "fifo_reader", thr_sync_evt);
}
+ if (writer)
+ inc_nwriters ();
}
void
@@ -1737,8 +1692,6 @@ fhandler_fifo::set_close_on_exec (bool val)
set_no_inheritance (owner_needed_evt, val);
set_no_inheritance (owner_found_evt, val);
set_no_inheritance (update_needed_evt, val);
- set_no_inheritance (check_write_ready_evt, val);
- set_no_inheritance (write_ready_ok_evt, val);
fifo_client_lock ();
for (int i = 0; i < nhandlers; i++)
set_no_inheritance (fc_handler[i].h, val);
--
2.27.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 03/12] Cygwin: fhandler_fifo::hit_eof: improve reliability
2020-07-16 16:19 [PATCH 00/12] FIFO: fix multiple reader support Ken Brown
2020-07-16 16:19 ` [PATCH 01/12] Cygwin: FIFO: fix problems finding new owner Ken Brown
2020-07-16 16:19 ` [PATCH 02/12] Cygwin: FIFO: keep a writer count in shared memory Ken Brown
@ 2020-07-16 16:19 ` Ken Brown
2020-07-16 16:19 ` [PATCH 04/12] Cygwin: FIFO: reduce I/O interleaving Ken Brown
` (9 subsequent siblings)
12 siblings, 0 replies; 14+ messages in thread
From: Ken Brown @ 2020-07-16 16:19 UTC (permalink / raw)
To: cygwin-patches
Use the writer count introduced in the previous commit to help detect
EOF. Drop the maybe_eof method, which is no longer needed.
---
winsup/cygwin/fhandler.h | 7 +++----
winsup/cygwin/fhandler_fifo.cc | 26 ++------------------------
winsup/cygwin/select.cc | 3 +--
3 files changed, 6 insertions(+), 30 deletions(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index f034a110e..b5bfdd0b3 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1392,7 +1392,6 @@ class fhandler_fifo: public fhandler_base
UNICODE_STRING pipe_name;
WCHAR pipe_name_buf[CYGWIN_FIFO_PIPE_NAME_LEN + 1];
- bool _maybe_eof;
fifo_client_handler *fc_handler; /* Dynamically growing array. */
int shandlers; /* Size (capacity) of the array. */
int nhandlers; /* Number of elements in the array. */
@@ -1473,9 +1472,9 @@ class fhandler_fifo: public fhandler_base
public:
fhandler_fifo ();
- bool hit_eof ();
- bool maybe_eof () const { return _maybe_eof; }
- void maybe_eof (bool val) { _maybe_eof = val; }
+ /* Called if we appear to be at EOF after polling fc_handlers. */
+ bool hit_eof () const
+ { return !nwriters () && !IsEventSignalled (writer_opening); }
int get_nhandlers () const { return nhandlers; }
fifo_client_handler &get_fc_handler (int i) { return fc_handler[i]; }
PUNICODE_STRING get_pipe_name ();
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 26b24d019..3685cc0c2 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -87,7 +87,7 @@ fhandler_fifo::fhandler_fifo ():
fhandler_base (),
read_ready (NULL), write_ready (NULL), writer_opening (NULL),
owner_needed_evt (NULL), owner_found_evt (NULL), update_needed_evt (NULL),
- cancel_evt (NULL), thr_sync_evt (NULL), _maybe_eof (false),
+ cancel_evt (NULL), thr_sync_evt (NULL),
fc_handler (NULL), shandlers (0), nhandlers (0),
reader (false), writer (false), duplexer (false),
max_atomic_write (DEFAULT_PIPEBUFSIZE),
@@ -361,8 +361,6 @@ fhandler_fifo::record_connection (fifo_client_handler& fc,
fifo_client_connect_state s)
{
fc.state = s;
- maybe_eof (false);
- ResetEvent (writer_opening);
set_pipe_non_blocking (fc.h, true);
}
@@ -1173,25 +1171,6 @@ fhandler_fifo::raw_write (const void *ptr, size_t len)
return ret;
}
-/* A reader is at EOF if the pipe is empty and no writers are open.
- hit_eof is called by raw_read and select.cc:peek_fifo if it appears
- that we are at EOF after polling the fc_handlers. We recheck this
- in case a writer opened while we were polling. */
-bool
-fhandler_fifo::hit_eof ()
-{
- bool ret = maybe_eof () && !IsEventSignalled (writer_opening);
- if (ret)
- {
- yield ();
- /* Wait for the reader thread to finish recording any connection. */
- fifo_client_lock ();
- fifo_client_unlock ();
- ret = maybe_eof ();
- }
- return ret;
-}
-
/* Called from raw_read and select.cc:peek_fifo. */
void
fhandler_fifo::take_ownership ()
@@ -1261,9 +1240,8 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
break;
}
}
- maybe_eof (!nconnected && !IsEventSignalled (writer_opening));
fifo_client_unlock ();
- if (maybe_eof () && hit_eof ())
+ if (!nconnected && hit_eof ())
{
reading_unlock ();
len = 0;
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 9ae567c38..80d16f2a7 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -883,9 +883,8 @@ peek_fifo (select_record *s, bool from_select)
goto out;
}
}
- fh->maybe_eof (!nconnected);
fh->fifo_client_unlock ();
- if (fh->maybe_eof () && fh->hit_eof ())
+ if (!nconnected && fh->hit_eof ())
{
select_printf ("read: %s, saw EOF", fh->get_name ());
gotone += s->read_ready = true;
--
2.27.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 04/12] Cygwin: FIFO: reduce I/O interleaving
2020-07-16 16:19 [PATCH 00/12] FIFO: fix multiple reader support Ken Brown
` (2 preceding siblings ...)
2020-07-16 16:19 ` [PATCH 03/12] Cygwin: fhandler_fifo::hit_eof: improve reliability Ken Brown
@ 2020-07-16 16:19 ` Ken Brown
2020-07-16 16:19 ` [PATCH 05/12] Cygwin: FIFO: improve taking ownership in fifo_reader_thread Ken Brown
` (8 subsequent siblings)
12 siblings, 0 replies; 14+ messages in thread
From: Ken Brown @ 2020-07-16 16:19 UTC (permalink / raw)
To: cygwin-patches
Add a bool member 'last_read' to the fifo_client_handler structure,
which is set to true on a successful read. This is used by raw_read
as follows.
When raw_read is called, it first locates the writer (if any) for
which last_read is true. raw_read tries to read from that writer and
returns if there is input available. Otherwise, it proceeds to poll
all the writers, as before.
The effect of this is that if a writer writes some data that is only
partially read, the next attempt to read will continue to read from
the same writer. This should reduce the interleaving of output from
different writers.
---
winsup/cygwin/fhandler.h | 3 +-
winsup/cygwin/fhandler_fifo.cc | 55 +++++++++++++++++++++++++++++-----
2 files changed, 50 insertions(+), 8 deletions(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index b5bfdd0b3..221c856e6 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1298,7 +1298,8 @@ struct fifo_client_handler
{
HANDLE h;
fifo_client_connect_state state;
- fifo_client_handler () : h (NULL), state (fc_unknown) {}
+ bool last_read; /* true if our last successful read was from this client. */
+ fifo_client_handler () : h (NULL), state (fc_unknown), last_read (false) {}
void close () { NtClose (h); }
fifo_client_connect_state set_state ();
};
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 3685cc0c2..afe21a468 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -404,6 +404,7 @@ fhandler_fifo::update_my_handlers ()
goto out;
}
fc.state = shared_fc_handler[i].state;
+ fc.last_read = shared_fc_handler[i].last_read;
}
out:
set_prev_owner (null_fr_id);
@@ -1200,15 +1201,56 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
/* No one else can take ownership while we hold the reading_lock. */
reading_lock ();
take_ownership ();
- /* Poll the connected clients for input. */
- int nconnected = 0;
+ /* 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 ();
+ /* First pass. */
+ int j;
+ for (j = 0; j < nhandlers; j++)
+ if (fc_handler[j].last_read)
+ break;
+ if (j < nhandlers && fc_handler[j].state >= fc_closing)
+ {
+ NTSTATUS status;
+ IO_STATUS_BLOCK io;
+
+ status = NtReadFile (fc_handler[j].h, NULL, NULL, NULL,
+ &io, in_ptr, len, NULL, NULL);
+ switch (status)
+ {
+ case STATUS_SUCCESS:
+ case STATUS_BUFFER_OVERFLOW:
+ /* io.Information is supposedly valid in latter case. */
+ if (io.Information > 0)
+ {
+ len = io.Information;
+ fifo_client_unlock ();
+ reading_unlock ();
+ return;
+ }
+ break;
+ case STATUS_PIPE_EMPTY:
+ break;
+ case STATUS_PIPE_BROKEN:
+ fc_handler[j].state = fc_disconnected;
+ break;
+ default:
+ debug_printf ("NtReadFile status %y", status);
+ fc_handler[j].state = fc_error;
+ break;
+ }
+ fc_handler[j].last_read = false;
+ }
+
+ /* Second pass. */
+ int nconnected = 0;
for (int i = 0; i < nhandlers; i++)
if (fc_handler[i].state >= fc_closing)
{
NTSTATUS status;
IO_STATUS_BLOCK io;
- size_t nbytes = 0;
nconnected++;
status = NtReadFile (fc_handler[i].h, NULL, NULL, NULL,
@@ -1217,11 +1259,10 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
{
case STATUS_SUCCESS:
case STATUS_BUFFER_OVERFLOW:
- /* io.Information is supposedly valid. */
- nbytes = io.Information;
- if (nbytes > 0)
+ if (io.Information > 0)
{
- len = nbytes;
+ len = io.Information;
+ fc_handler[i].last_read = true;
fifo_client_unlock ();
reading_unlock ();
return;
--
2.27.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 05/12] Cygwin: FIFO: improve taking ownership in fifo_reader_thread
2020-07-16 16:19 [PATCH 00/12] FIFO: fix multiple reader support Ken Brown
` (3 preceding siblings ...)
2020-07-16 16:19 ` [PATCH 04/12] Cygwin: FIFO: reduce I/O interleaving Ken Brown
@ 2020-07-16 16:19 ` Ken Brown
2020-07-16 16:19 ` [PATCH 06/12] Cygwin: FIFO: fix indentation Ken Brown
` (7 subsequent siblings)
12 siblings, 0 replies; 14+ messages in thread
From: Ken Brown @ 2020-07-16 16:19 UTC (permalink / raw)
To: cygwin-patches
When a reader takes ownership in fifo_reader_thread, it now goes
directly to the part of the main loop that listens for a connection.
Previously it went back to the beginning of the loop.
Also, if the reader has to delay taking ownership because the previous
owner has not finished updating the shared fifo_client handlers, it
now checks to see if cancel_evt has been set. Previously it might
have had to spin its wheels unnecessarily only to eventually find that
its thread had been canceled.
---
winsup/cygwin/fhandler_fifo.cc | 44 ++++++++++++++++++----------------
1 file changed, 23 insertions(+), 21 deletions(-)
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index afe21a468..1fb319fcf 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -462,12 +462,30 @@ fhandler_fifo::fifo_reader_thread_func ()
take_ownership = true;
else if (cur_owner != me)
idle = true;
- if (take_ownership)
+ else
+ /* I'm the owner. */
+ goto owner_listen;
+ if (idle)
+ {
+ owner_unlock ();
+ HANDLE w[2] = { owner_needed_evt, cancel_evt };
+ switch (WaitForMultipleObjects (2, w, false, INFINITE))
+ {
+ case WAIT_OBJECT_0:
+ continue;
+ case WAIT_OBJECT_0 + 1:
+ goto canceled;
+ default:
+ api_fatal ("WFMO failed, %E");
+ }
+ }
+ else if (take_ownership)
{
if (!shared_fc_handler_updated ())
{
owner_unlock ();
- yield ();
+ if (IsEventSignalled (cancel_evt))
+ goto canceled;
continue;
}
else
@@ -478,26 +496,11 @@ fhandler_fifo::fifo_reader_thread_func ()
api_fatal ("Can't update my handlers, %E");
owner_found ();
owner_unlock ();
- continue;
+ /* Fall through to owner_listen. */
}
}
- else if (idle)
- {
- owner_unlock ();
- HANDLE w[2] = { owner_needed_evt, cancel_evt };
- switch (WaitForMultipleObjects (2, w, false, INFINITE))
- {
- case WAIT_OBJECT_0:
- continue;
- case WAIT_OBJECT_0 + 1:
- goto canceled;
- default:
- api_fatal ("WFMO failed, %E");
- }
- }
- else
- {
- /* I'm the owner */
+
+owner_listen:
fifo_client_lock ();
cleanup_handlers ();
if (add_client_handler () < 0)
@@ -590,7 +593,6 @@ fhandler_fifo::fifo_reader_thread_func ()
fifo_client_unlock ();
if (cancel)
goto canceled;
- }
}
canceled:
if (conn_evt)
--
2.27.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 06/12] Cygwin: FIFO: fix indentation
2020-07-16 16:19 [PATCH 00/12] FIFO: fix multiple reader support Ken Brown
` (4 preceding siblings ...)
2020-07-16 16:19 ` [PATCH 05/12] Cygwin: FIFO: improve taking ownership in fifo_reader_thread Ken Brown
@ 2020-07-16 16:19 ` Ken Brown
2020-07-16 16:19 ` [PATCH 07/12] Cygwin: FIFO: make certain errors non-fatal Ken Brown
` (6 subsequent siblings)
12 siblings, 0 replies; 14+ messages in thread
From: Ken Brown @ 2020-07-16 16:19 UTC (permalink / raw)
To: cygwin-patches
---
winsup/cygwin/fhandler_fifo.cc | 168 ++++++++++++++++-----------------
1 file changed, 84 insertions(+), 84 deletions(-)
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 1fb319fcf..69dda0811 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -501,98 +501,98 @@ fhandler_fifo::fifo_reader_thread_func ()
}
owner_listen:
- fifo_client_lock ();
- cleanup_handlers ();
- if (add_client_handler () < 0)
- api_fatal ("Can't add a client handler, %E");
-
- /* Listen for a writer to connect to the new client handler. */
- fifo_client_handler& fc = fc_handler[nhandlers - 1];
- fifo_client_unlock ();
- shared_fc_handler_updated (false);
- owner_unlock ();
- NTSTATUS status;
- IO_STATUS_BLOCK io;
- bool cancel = false;
- bool update = false;
+ fifo_client_lock ();
+ cleanup_handlers ();
+ if (add_client_handler () < 0)
+ api_fatal ("Can't add a client handler, %E");
- status = NtFsControlFile (fc.h, conn_evt, NULL, NULL, &io,
- FSCTL_PIPE_LISTEN, NULL, 0, NULL, 0);
- if (status == STATUS_PENDING)
- {
- HANDLE w[3] = { conn_evt, update_needed_evt, cancel_evt };
- switch (WaitForMultipleObjects (3, w, false, INFINITE))
- {
- case WAIT_OBJECT_0:
- status = io.Status;
- debug_printf ("NtFsControlFile STATUS_PENDING, then %y",
- status);
- break;
- case WAIT_OBJECT_0 + 1:
- status = STATUS_WAIT_1;
- update = true;
- break;
- case WAIT_OBJECT_0 + 2:
- status = STATUS_THREAD_IS_TERMINATING;
- cancel = true;
- update = true;
- break;
- default:
- api_fatal ("WFMO failed, %E");
- }
- }
- else
- debug_printf ("NtFsControlFile status %y, no STATUS_PENDING",
- status);
- HANDLE ph = NULL;
- NTSTATUS status1;
+ /* Listen for a writer to connect to the new client handler. */
+ fifo_client_handler& fc = fc_handler[nhandlers - 1];
+ fifo_client_unlock ();
+ shared_fc_handler_updated (false);
+ owner_unlock ();
+ NTSTATUS status;
+ IO_STATUS_BLOCK io;
+ bool cancel = false;
+ bool update = false;
- fifo_client_lock ();
- switch (status)
+ status = NtFsControlFile (fc.h, conn_evt, NULL, NULL, &io,
+ FSCTL_PIPE_LISTEN, NULL, 0, NULL, 0);
+ if (status == STATUS_PENDING)
+ {
+ HANDLE w[3] = { conn_evt, update_needed_evt, cancel_evt };
+ switch (WaitForMultipleObjects (3, w, false, INFINITE))
{
- case STATUS_SUCCESS:
- case STATUS_PIPE_CONNECTED:
- record_connection (fc);
+ case WAIT_OBJECT_0:
+ status = io.Status;
+ debug_printf ("NtFsControlFile STATUS_PENDING, then %y",
+ status);
break;
- case STATUS_PIPE_CLOSING:
- record_connection (fc, fc_closing);
+ case WAIT_OBJECT_0 + 1:
+ status = STATUS_WAIT_1;
+ update = true;
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:
- record_connection (fc, fc_closing);
- break;
- default:
- debug_printf ("NtFsControlFile status %y after failing to connect bogus client or real client", io.Status);
- fc.state = fc_unknown;
- break;
- }
+ case WAIT_OBJECT_0 + 2:
+ status = STATUS_THREAD_IS_TERMINATING;
+ cancel = true;
+ update = true;
break;
default:
- break;
+ api_fatal ("WFMO failed, %E");
}
- if (ph)
- NtClose (ph);
- if (update && update_shared_handlers () < 0)
- api_fatal ("Can't update shared handlers, %E");
- fifo_client_unlock ();
- if (cancel)
- goto canceled;
+ }
+ else
+ debug_printf ("NtFsControlFile status %y, no STATUS_PENDING",
+ status);
+ HANDLE ph = NULL;
+ NTSTATUS status1;
+
+ fifo_client_lock ();
+ switch (status)
+ {
+ case STATUS_SUCCESS:
+ case STATUS_PIPE_CONNECTED:
+ record_connection (fc);
+ break;
+ case STATUS_PIPE_CLOSING:
+ record_connection (fc, fc_closing);
+ 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:
+ record_connection (fc, fc_closing);
+ break;
+ default:
+ debug_printf ("NtFsControlFile status %y after failing to connect bogus client or real client", io.Status);
+ fc.state = fc_unknown;
+ break;
+ }
+ break;
+ default:
+ break;
+ }
+ if (ph)
+ NtClose (ph);
+ if (update && update_shared_handlers () < 0)
+ api_fatal ("Can't update shared handlers, %E");
+ fifo_client_unlock ();
+ if (cancel)
+ goto canceled;
}
canceled:
if (conn_evt)
--
2.27.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 07/12] Cygwin: FIFO: make certain errors non-fatal
2020-07-16 16:19 [PATCH 00/12] FIFO: fix multiple reader support Ken Brown
` (5 preceding siblings ...)
2020-07-16 16:19 ` [PATCH 06/12] Cygwin: FIFO: fix indentation Ken Brown
@ 2020-07-16 16:19 ` Ken Brown
2020-07-16 16:19 ` [PATCH 08/12] Cygwin: FIFO: add missing lock Ken Brown
` (5 subsequent siblings)
12 siblings, 0 replies; 14+ messages in thread
From: Ken Brown @ 2020-07-16 16:19 UTC (permalink / raw)
To: cygwin-patches
If update_my_handlers fails to duplicate one or more handles, just
mark the corresponding handlers as being in an error state.
But if update_my_handlers is unable to open the process of the
previous owner, it's likely that something serious has gone wrong, so
we continue to make that a fatal error.
---
winsup/cygwin/fhandler_fifo.cc | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 69dda0811..91a276ee9 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -383,11 +383,7 @@ fhandler_fifo::update_my_handlers ()
else
prev_proc = OpenProcess (PROCESS_DUP_HANDLE, false, prev.winpid);
if (!prev_proc)
- {
- debug_printf ("Can't open process of previous owner, %E");
- __seterrno ();
- goto out;
- }
+ api_fatal ("Can't open process of previous owner, %E");
for (int i = 0; i < get_shared_nhandlers (); i++)
{
@@ -399,14 +395,17 @@ fhandler_fifo::update_my_handlers ()
!close_on_exec (), DUPLICATE_SAME_ACCESS))
{
debug_printf ("Can't duplicate handle of previous owner, %E");
- --nhandlers;
__seterrno ();
- goto out;
+ fc.state = fc_error;
+ fc.last_read = false;
+ ret = -1;
+ }
+ else
+ {
+ fc.state = shared_fc_handler[i].state;
+ fc.last_read = shared_fc_handler[i].last_read;
}
- fc.state = shared_fc_handler[i].state;
- fc.last_read = shared_fc_handler[i].last_read;
}
-out:
set_prev_owner (null_fr_id);
return ret;
}
@@ -493,7 +492,7 @@ fhandler_fifo::fifo_reader_thread_func ()
set_owner (me);
set_pending_owner (null_fr_id);
if (update_my_handlers () < 0)
- api_fatal ("Can't update my handlers, %E");
+ debug_printf ("error updating my handlers, %E");
owner_found ();
owner_unlock ();
/* Fall through to owner_listen. */
--
2.27.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 08/12] Cygwin: FIFO: add missing lock
2020-07-16 16:19 [PATCH 00/12] FIFO: fix multiple reader support Ken Brown
` (6 preceding siblings ...)
2020-07-16 16:19 ` [PATCH 07/12] Cygwin: FIFO: make certain errors non-fatal Ken Brown
@ 2020-07-16 16:19 ` Ken Brown
2020-07-16 16:19 ` [PATCH 09/12] Cygwin: fhandler_fifo::take_ownership: don't set event unnecessarily Ken Brown
` (4 subsequent siblings)
12 siblings, 0 replies; 14+ messages in thread
From: Ken Brown @ 2020-07-16 16:19 UTC (permalink / raw)
To: cygwin-patches
---
winsup/cygwin/fhandler_fifo.cc | 2 ++
1 file changed, 2 insertions(+)
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 91a276ee9..b6e172ddc 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -422,7 +422,9 @@ 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;
}
--
2.27.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 09/12] Cygwin: fhandler_fifo::take_ownership: don't set event unnecessarily
2020-07-16 16:19 [PATCH 00/12] FIFO: fix multiple reader support Ken Brown
` (7 preceding siblings ...)
2020-07-16 16:19 ` [PATCH 08/12] Cygwin: FIFO: add missing lock Ken Brown
@ 2020-07-16 16:19 ` Ken Brown
2020-07-16 16:19 ` [PATCH 10/12] Cygwin: FIFO: allow take_ownership to be interrupted Ken Brown
` (3 subsequent siblings)
12 siblings, 0 replies; 14+ messages in thread
From: Ken Brown @ 2020-07-16 16:19 UTC (permalink / raw)
To: cygwin-patches
Don't set update_needed_evt if there's currently no owner. This will
cause unnecessary churn once I'm the owner and am listening for
connections.
---
winsup/cygwin/fhandler_fifo.cc | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index b6e172ddc..fd1695f40 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -1186,8 +1186,11 @@ fhandler_fifo::take_ownership ()
return;
}
set_pending_owner (me);
+ /* Wake up my fifo_reader_thread. */
owner_needed ();
- SetEvent (update_needed_evt);
+ if (get_owner ())
+ /* Wake up owner's fifo_reader_thread. */
+ SetEvent (update_needed_evt);
owner_unlock ();
/* The reader threads should now do the transfer. */
WaitForSingleObject (owner_found_evt, INFINITE);
--
2.27.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 10/12] Cygwin: FIFO: allow take_ownership to be interrupted
2020-07-16 16:19 [PATCH 00/12] FIFO: fix multiple reader support Ken Brown
` (8 preceding siblings ...)
2020-07-16 16:19 ` [PATCH 09/12] Cygwin: fhandler_fifo::take_ownership: don't set event unnecessarily Ken Brown
@ 2020-07-16 16:19 ` Ken Brown
2020-07-16 16:19 ` [PATCH 11/12] Cygwin: FIFO: clean up Ken Brown
` (2 subsequent siblings)
12 siblings, 0 replies; 14+ messages in thread
From: Ken Brown @ 2020-07-16 16:19 UTC (permalink / raw)
To: cygwin-patches
Use cygwait in take_ownership to allow interruption while waiting to
become owner. Return the cygwait return value or a suitable value to
indicate an error.
raw_read now checks the return value and acts accordingly.
---
winsup/cygwin/fhandler.h | 2 +-
winsup/cygwin/fhandler_fifo.cc | 54 ++++++++++++++++++++++++++++++----
winsup/cygwin/select.cc | 11 ++++++-
3 files changed, 59 insertions(+), 8 deletions(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 221c856e6..0e0cfbd71 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1489,7 +1489,7 @@ public:
void owner_lock () { shmem->owner_lock (); }
void owner_unlock () { shmem->owner_unlock (); }
- void take_ownership ();
+ DWORD take_ownership ();
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 fd1695f40..30486304f 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -1175,15 +1175,16 @@ fhandler_fifo::raw_write (const void *ptr, size_t len)
return ret;
}
-/* Called from raw_read and select.cc:peek_fifo. */
-void
+/* Called from raw_read and select.cc:peek_fifo. Return WAIT_OBJECT_0
+ on success. */
+DWORD
fhandler_fifo::take_ownership ()
{
owner_lock ();
if (get_owner () == me)
{
owner_unlock ();
- return;
+ return WAIT_OBJECT_0;
}
set_pending_owner (me);
/* Wake up my fifo_reader_thread. */
@@ -1192,8 +1193,19 @@ fhandler_fifo::take_ownership ()
/* Wake up owner's fifo_reader_thread. */
SetEvent (update_needed_evt);
owner_unlock ();
- /* The reader threads should now do the transfer. */
- WaitForSingleObject (owner_found_evt, INFINITE);
+ /* 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 ()))
+ {
+ /* 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;
+ }
+ owner_unlock ();
+ return waitret;
}
void __reg3
@@ -1206,7 +1218,37 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
{
/* No one else can take ownership while we hold the reading_lock. */
reading_lock ();
- take_ownership ();
+ 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;
+ }
+
/* 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
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 80d16f2a7..3f3f33fb5 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -867,7 +867,16 @@ peek_fifo (select_record *s, bool from_select)
}
fh->reading_lock ();
- fh->take_ownership ();
+ if (fh->take_ownership () != WAIT_OBJECT_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.27.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 11/12] Cygwin: FIFO: clean up
2020-07-16 16:19 [PATCH 00/12] FIFO: fix multiple reader support Ken Brown
` (9 preceding siblings ...)
2020-07-16 16:19 ` [PATCH 10/12] Cygwin: FIFO: allow take_ownership to be interrupted Ken Brown
@ 2020-07-16 16:19 ` Ken Brown
2020-07-16 16:19 ` [PATCH 12/12] Cygwin: FIFO: update commentary Ken Brown
2020-07-16 19:57 ` [PATCH 00/12] FIFO: fix multiple reader support Corinna Vinschen
12 siblings, 0 replies; 14+ messages in thread
From: Ken Brown @ 2020-07-16 16:19 UTC (permalink / raw)
To: cygwin-patches
Remove the fhandler_fifo::get_me method, which is no longer used.
Make the methods get_owner, set_owner, owner_lock, and owner_unlock
private.
---
winsup/cygwin/fhandler.h | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 0e0cfbd71..60bd27e00 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1437,6 +1437,8 @@ class fhandler_fifo: public fhandler_base
void nwriters_lock () { shmem->nwriters_lock (); }
void nwriters_unlock () { shmem->nwriters_unlock (); }
+ fifo_reader_id_t get_owner () const { return shmem->get_owner (); }
+ void set_owner (fifo_reader_id_t fr_id) { shmem->set_owner (fr_id); }
fifo_reader_id_t get_prev_owner () const { return shmem->get_prev_owner (); }
void set_prev_owner (fifo_reader_id_t fr_id)
{ shmem->set_prev_owner (fr_id); }
@@ -1444,6 +1446,8 @@ class fhandler_fifo: public fhandler_base
{ return shmem->get_pending_owner (); }
void set_pending_owner (fifo_reader_id_t fr_id)
{ shmem->set_pending_owner (fr_id); }
+ void owner_lock () { shmem->owner_lock (); }
+ void owner_unlock () { shmem->owner_unlock (); }
void owner_needed ()
{
@@ -1483,12 +1487,6 @@ public:
void fifo_client_lock () { _fifo_client_lock.lock (); }
void fifo_client_unlock () { _fifo_client_lock.unlock (); }
- fifo_reader_id_t get_me () const { return me; }
- fifo_reader_id_t get_owner () const { return shmem->get_owner (); }
- void set_owner (fifo_reader_id_t fr_id) { shmem->set_owner (fr_id); }
- void owner_lock () { shmem->owner_lock (); }
- void owner_unlock () { shmem->owner_unlock (); }
-
DWORD take_ownership ();
void reading_lock () { shmem->reading_lock (); }
void reading_unlock () { shmem->reading_unlock (); }
--
2.27.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 12/12] Cygwin: FIFO: update commentary
2020-07-16 16:19 [PATCH 00/12] FIFO: fix multiple reader support Ken Brown
` (10 preceding siblings ...)
2020-07-16 16:19 ` [PATCH 11/12] Cygwin: FIFO: clean up Ken Brown
@ 2020-07-16 16:19 ` Ken Brown
2020-07-16 19:57 ` [PATCH 00/12] FIFO: fix multiple reader support Corinna Vinschen
12 siblings, 0 replies; 14+ messages in thread
From: Ken Brown @ 2020-07-16 16:19 UTC (permalink / raw)
To: cygwin-patches
---
winsup/cygwin/fhandler_fifo.cc | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 30486304f..e9d0187d4 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -52,10 +52,23 @@
which is mostly idle. The thread wakes up if that reader might
need to take ownership.
- There is a block of shared memory, accessible to all readers,
- that contains information needed for the owner change process.
- It also contains some locks to prevent races and deadlocks
- between the various threads.
+ There is a block of named shared memory, accessible to all
+ fhandlers for a given FIFO. It keeps track of the number of open
+ readers and writers; it contains information needed for the owner
+ change process; and it contains some locks to prevent races and
+ deadlocks between the various threads.
+
+ The shared memory is created by the first reader to open the
+ FIFO. It is opened by subsequent readers and by all writers. It
+ is destroyed by Windows when the last handle to it is closed.
+
+ If a handle to it somehow remains open after all processes
+ holding file descriptors to the FIFO have closed, the shared
+ memory can persist and be reused with stale data by the next
+ process that opens the FIFO. So far I've seen this happen only
+ as a result of a bug in the code, but there are some debug_printf
+ statements in fhandler_fifo::open to help detect this if it
+ happens again.
At this writing, I know of only one application (Midnight
Commander when running under tcsh) that *explicitly* opens two
--
2.27.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 00/12] FIFO: fix multiple reader support
2020-07-16 16:19 [PATCH 00/12] FIFO: fix multiple reader support Ken Brown
` (11 preceding siblings ...)
2020-07-16 16:19 ` [PATCH 12/12] Cygwin: FIFO: update commentary Ken Brown
@ 2020-07-16 19:57 ` Corinna Vinschen
12 siblings, 0 replies; 14+ messages in thread
From: Corinna Vinschen @ 2020-07-16 19:57 UTC (permalink / raw)
To: cygwin-patches
Hi Ken,
On Jul 16 12:19, Ken Brown via Cygwin-patches wrote:
> There were several flaws in my previous attempt to add support for
> explicitly opening a FIFO multiple times for reading. (By
> "explicitly" I mean by calling open rather than by calling
> fork/exec/dup.) See
>
> https://sourceware.org/pipermail/cygwin/2020-July/245456.html
>
> for one indication of problems
>
> The most important flaw was that I tried to use an indirect,
> unreliable method for determining whether there are writers open.
> This is fixed in the second patch of this series by adding a member
> '_nwriters' to struct fifo_shmem_t, which counts the number of open
> writers.
>
> We now have to give writers access to the shared memory as well as
> readers, so that they can increment _nwriters in open/fork/exec/dup
> and decrement it in close.
>
> The other patches contain miscellaneous fixes/improvements.
>
> Ken Brown (12):
> Cygwin: FIFO: fix problems finding new owner
> Cygwin: FIFO: keep a writer count in shared memory
> Cygwin: fhandler_fifo::hit_eof: improve reliability
> Cygwin: FIFO: reduce I/O interleaving
> Cygwin: FIFO: improve taking ownership in fifo_reader_thread
> Cygwin: FIFO: fix indentation
> Cygwin: FIFO: make certain errors non-fatal
> Cygwin: FIFO: add missing lock
> Cygwin: fhandler_fifo::take_ownership: don't set event unnecessarily
> Cygwin: FIFO: allow take_ownership to be interrupted
> Cygwin: FIFO: clean up
> Cygwin: FIFO: update commentary
>
> winsup/cygwin/fhandler.h | 55 +--
> winsup/cygwin/fhandler_fifo.cc | 725 ++++++++++++++++++---------------
> winsup/cygwin/select.cc | 14 +-
> 3 files changed, 433 insertions(+), 361 deletions(-)
LGTM, please push.
Thanks,
Corinna
--
Corinna Vinschen
Cygwin Maintainer
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-07-16 19:57 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-16 16:19 [PATCH 00/12] FIFO: fix multiple reader support Ken Brown
2020-07-16 16:19 ` [PATCH 01/12] Cygwin: FIFO: fix problems finding new owner Ken Brown
2020-07-16 16:19 ` [PATCH 02/12] Cygwin: FIFO: keep a writer count in shared memory Ken Brown
2020-07-16 16:19 ` [PATCH 03/12] Cygwin: fhandler_fifo::hit_eof: improve reliability Ken Brown
2020-07-16 16:19 ` [PATCH 04/12] Cygwin: FIFO: reduce I/O interleaving Ken Brown
2020-07-16 16:19 ` [PATCH 05/12] Cygwin: FIFO: improve taking ownership in fifo_reader_thread Ken Brown
2020-07-16 16:19 ` [PATCH 06/12] Cygwin: FIFO: fix indentation Ken Brown
2020-07-16 16:19 ` [PATCH 07/12] Cygwin: FIFO: make certain errors non-fatal Ken Brown
2020-07-16 16:19 ` [PATCH 08/12] Cygwin: FIFO: add missing lock Ken Brown
2020-07-16 16:19 ` [PATCH 09/12] Cygwin: fhandler_fifo::take_ownership: don't set event unnecessarily Ken Brown
2020-07-16 16:19 ` [PATCH 10/12] Cygwin: FIFO: allow take_ownership to be interrupted Ken Brown
2020-07-16 16:19 ` [PATCH 11/12] Cygwin: FIFO: clean up Ken Brown
2020-07-16 16:19 ` [PATCH 12/12] Cygwin: FIFO: update commentary Ken Brown
2020-07-16 19:57 ` [PATCH 00/12] FIFO: fix multiple reader support 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).