Commit Graph

157 Commits

Author SHA1 Message Date
Ken Brown 0fda55133a Cygwin: FIFO: synchronize the fifo_reader and fifosel threads
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.
2020-08-04 08:15:19 -04:00
Ken Brown 251624a352 Cygwin: FIFO: don't read from pipes that are closing
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.
2020-08-04 08:15:19 -04:00
Ken Brown 289af73a89 Cygwin: FIFO: reorganize some fifo_client_handler methods
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().
2020-08-04 08:15:19 -04:00
Ken Brown 6ed067a0ae Cygwin: FIFO: add a timeout to take_ownership
fhandler_fifo::take_ownership() is called from select.cc::peek_fifo
and fhandler_fifo::raw_read and could potentially block indefinitely
if something goes wrong.  This is always undesirable in peek_fifo, and
it is undesirable in a nonblocking read.  Fix this by adding a timeout
parameter to take_ownership.

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

Replace the call to cygwait in take_ownership by a call to WFSO.
There's no need to allow interruption now that we have a timeout.
2020-08-04 08:15:19 -04:00
Ken Brown 6acce025d0 Cygwin: FIFO: fix timing issue with owner change
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.
2020-08-04 08:02:07 -04:00
Ken Brown e319fd0e62 Cygwin: FIFO: lock fixes
Add some missing locks and remove one extra unlock.  Clarify for some
functions whether caller or callee acquires lock, and add appropriate
comments.
2020-08-04 07:57:45 -04:00
Ken Brown ac371ee1ba Cygwin: FIFO: update commentary 2020-07-16 15:59:53 -04:00
Ken Brown 4eaa55463d Cygwin: FIFO: allow take_ownership to be interrupted
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.
2020-07-16 15:59:53 -04:00
Ken Brown a4dc0eb15c Cygwin: fhandler_fifo::take_ownership: don't set event unnecessarily
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.
2020-07-16 15:59:53 -04:00
Ken Brown 4f25d82cb1 Cygwin: FIFO: add missing lock 2020-07-16 15:59:53 -04:00
Ken Brown d3a01b7ec2 Cygwin: FIFO: make certain errors non-fatal
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.
2020-07-16 15:59:53 -04:00
Ken Brown b0418138fe Cygwin: FIFO: fix indentation 2020-07-16 15:59:53 -04:00
Ken Brown 6b8a829496 Cygwin: FIFO: improve taking ownership in fifo_reader_thread
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.
2020-07-16 15:59:53 -04:00
Ken Brown 1c0cf5f4f9 Cygwin: FIFO: reduce I/O interleaving
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.
2020-07-16 15:59:53 -04:00
Ken Brown e10425e1e3 Cygwin: fhandler_fifo::hit_eof: improve reliability
Use the writer count introduced in the previous commit to help detect
EOF.  Drop the maybe_eof method, which is no longer needed.
2020-07-16 15:59:53 -04:00
Ken Brown 8ca713d70a Cygwin: FIFO: keep a writer count in shared memory
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.
2020-07-16 15:59:53 -04:00
Ken Brown da9fea0759 Cygwin: FIFO: fix problems finding new owner
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.
2020-07-16 15:59:53 -04:00
Ken Brown bf07202e16 Cygwin: FIFO: add missing unlock
There was a missing call to reader_opening_unlock on one of the error
exits in fhandler_fifo::open.
2020-05-22 10:30:54 -04:00
Ken Brown fe937e21ad Cygwin: FIFO: Revert "take ownership on exec"
This reverts commit 39a9cd9465.

There is no need to explicitly take ownership in fixup_after_exec; if
ownership transfer is needed, it will be taken care of by
fhandler_fifo::close when the parent closes.  Moreover, closing the
parent's fifo_reader_thread can cause problems, such as the one
reported here:

  https://cygwin.com/pipermail/cygwin-patches/2020q2/010235.html
2020-05-22 10:30:54 -04:00
Ken Brown e637d53617 Cygwin: FIFO: improve the interruptibility of raw_read
During a blocking read, we sleep for 1 ms after each iteration through
the connected writers.  Currently we do this by calling Sleep (1).
Remove this call to Sleep and instead change the timeout in the
cygwait call from 0 to 1, so that raw_read can be interrupted while
sleeping.
2020-05-11 09:52:23 -04:00
Ken Brown 1f27345947 Cygwin: FIFO: code simplification
There are currently three functions that call NtQueryInformationFile
to determine the state of a pipe instance.  Do this only once, in a
new fifo_client_handler::set_state () function, and call that when
state information is needed.

Remove the fifo_client_handler methods pipe_state and get_state, which
are no longer needed.

Make fhandler_fifo::get_fc_handler return a reference, for use in
select.cc:peek_fifo.

Make other small changes to ensure that this commit doesn't change any
decisions based on the state of a fifo_client_handler.

The tricky part is interpreting FILE_PIPE_CLOSING_STATE, which we
translate to fc_closing.  Our current interpretation, which is not
changing as a result of this commit, is that the writer at the other
end of the pipe instance is viewed as still connected from the point
of view of raw_read and determining EOF.

But it is not viewed as still connected if we are deciding whether to
unblock a new reader that is trying to open.
2020-05-11 09:52:16 -04:00
Ken Brown 98dfadec3a Cygwin: FIFO: update commentary
The beginning of fhandler_fifo.cc contains a long comment giving an
overview of the FIFO implementation.  This is now updated to describe
the support for multiple readers.
2020-05-08 06:45:24 -04:00
Ken Brown 4811889e0c Cygwin: FIFO: support opening multiple readers
Although we can have multiple readers open because of dup/fork/exec,
the current code does not support multiple readers opening a FIFO by
explicitly calling 'open'.

The main complication in supporting this is that when a blocking
reader tries to open and there's already one open, it has to check
whether there any writers open.  It can't rely on the write_ready
event, whose state hasn't changed since the first writer opened.

To fix this, add two new named events, check_write_ready_evt and
write_ready_ok_evt, and a new method, check_write_ready().

The first event signals the owner's reader thread to call
check_write_ready(), which polls the fc_handler list to check for
connected writers.  If it finds none, it checks to see if there's a
writer in the process and then sets/resets write_ready appropriately.

When check_write_ready() finishes it sets write_ready_ok_evt to signal
the reader that write_ready has been updated.

The polling is done via fifo_client_handler::pipe_state().  As long as
it's calling that function anyway, check_write_ready() updates the
state of each handler.

Also add a new lock to prevent a race if two readers are trying to
open simultaneously.
2020-05-08 06:45:24 -04:00
Ken Brown bf66a56cca Cygwin: FIFO: allow any reader to take ownership
Add a take_ownership method, used by raw_read and select.cc:peek_fifo.
It wakes up all fifo_reader_threads and allows the caller to become
owner.  The work is done by the fifo_reader_threads.

For synchronization we introduce several new fhandler_fifo data
members and methods:

- update_needed_evt signals the current owner to stop listening for
  writer connections and update its fc_handler list.

- shared_fc_handler() gets and sets the status of the fc_handler
  update process.

- get_pending_owner() and set_pending_owner() get and set the reader
  that is requesting ownership.

Finally, a new 'reading_lock' prevents two readers from trying to take
ownership simultaneously.
2020-05-08 06:45:24 -04:00
Ken Brown f35dfff3de Cygwin: FIFO: find a new owner when closing
If the owning reader is closing, wait for another reader (if there is
one) to take ownership before closing the owner's pipe handles.

To synchronize the ownership transfer, add events owner_needed_evt and
owner_found_evt, and add methods owner_needed and owner_found to
set/reset them.

Modify the fifo_reader_thread function to wake up all non-owners when
a new owner is needed.

Make a cosmetic change in close so that fhandler_base::close is called
only if we have a write handle.  This prevents strace output from
being littered with statements that the null handle is being closed.
2020-05-08 06:45:24 -04:00
Ken Brown 39a9cd9465 Cygwin: FIFO: take ownership on exec
If fixup_after_exec is called on a non-close-on-exec reader whose
parent is the owner, transfer ownership to the child.  Otherwise the
parent's pipe handles will be closed before any other reader can
duplicate them.

To help with this, make the cancel_evt and thr_sync_evt handles
inheritable, so that the child can terminate the parent's
fifo_reader_thread (and the parent will update the shared fc_handler
list).

Add an optional argument 'from_exec' to update_my_handlers to simplify
its use in this case; no handle duplication is required.
2020-05-08 06:45:20 -04:00
Ken Brown d9918451e2 Cygwin: FIFO: add a shared fifo_client_handler list
This is in a new shared memory section.  We will use it for temporary
storage of the owner's fc_handler list when we need to change owner.
The new owner can then duplicate the pipe handles from that list
before taking ownership.

Add several shared data members and methods that are needed for the
duplication process

Add methods update_my_handlers and update_shared_handlers that carry
out the duplication.

Allow the shared list to grow dynamically, up to a point.  Do this by
initially reserving a block of memory (currently 100 pages) and only
committing pages as needed.

Add methods create_shared_fc_handler, reopen_shared_fc_handler, and
remap_shared_fc_handler to create the new shared memory section,
reopen it, and commit new pages.  The first is called in open, the
second is called in dup/fork/exec, and the third is called in
update_shared_handlers if more shared memory is needed.

Modify the fifo_reader_thread function to call update_my_handlers when
it finds that there is no owner.  Also make it call
update_shared_handlers when the owner's thread terminates, so that the
new owner will have an accurate shared fc_handler list from which to
duplicate.

For convenience, add new methods cleanup_handlers and
close_all_handlers.  And add an optional arg to add_client_handler
that allows it to create a new fifo_client_handler without creating a
new pipe instance.
2020-05-08 06:36:31 -04:00
Ken Brown c76ded2ca0 Cygwin: FIFO: allow fc_handler list to grow dynamically
Make fc_handler a pointer to malloc'd memory instead of a fixed-size
array.  The size is now a new data member 'shandlers'.  Call realloc
in add_client_handler if we need to grow the array.

free fc_handler in close.  As long as we're touching that code, also
remove an unneeded lock.
2020-05-08 06:32:00 -04:00
Ken Brown 606baf5566 Cygwin: FIFO: designate one reader as owner
Among all the open readers of a FIFO, one is declared to be the owner.
This is the only reader that listens for client connections, and it is
the only one that has an accurate fc_handler list.

Add shared data and methods for getting and setting the owner, as well
as a lock to prevent more than one reader from accessing these data
simultaneously.

Modify the fifo_reader_thread so that it checks the owner at the
beginning of its loop.  If there is no owner, it takes ownership.  If
there is an owner but it is a different reader, the thread just waits
to be canceled.  Otherwise, it listens for client connections as
before.

Remove the 'first' argument from create_pipe_instance.  It is not
needed, and it may be confusing in the future since only the owner
knows whether a pipe instance is the first.

When opening a reader, don't return until the fifo_reader_thread has
time to set an owner.

If the owner closes, indicate that there is no longer an owner.

Clear the child's fc_handler list in dup, and don't bother duplicating
the handles.  The child never starts out as owner, so it can't use
those handles.

Do the same thing in fixup_after_fork in the close-on-exec case.  In
the non-close-on-exec case, the child inherits an fc_handler list that
it can't use, but we can just leave it alone; the handles will be
closed when the child is closed.
2020-05-08 06:32:00 -04:00
Ken Brown 16e7c10578 Cygwin: FIFO: introduce a new type, fifo_reader_id_t
This uniquely identifies an fhandler_fifo open for reading in any
process.

Add a new data member 'me' of this type, which is set in open, dup,
fork, and exec.
2020-05-08 06:32:00 -04:00
Ken Brown 365818a4a5 Cygwin: FIFO: keep track of the number of readers
Add data and methods to the shared memory that keep track of the
number of open readers.

Increment this number in open, dup, fork, and exec.  Decrement it in
close.  Reset read_ready if there are no readers left.
2020-05-08 06:32:00 -04:00
Ken Brown 878eb22462 Cygwin: FIFO: add shared memory
Even though we currently allow a FIFO to be opened for reading only
once, we can still have more than one reader open because of dup and
fork.  Add a named shared memory section accessible to all readers of
a given FIFO.  In future commits we will add information needed by all
readers to this section

Add a class fifo_shmem_t that lets us access this information.

Add a method create_shmem that is called when a reader opens, and add
a method reopen_shmem that is called by dup, fork, and exec.  (Each
new reader needs its own view of the shared memory.)
2020-05-08 06:32:00 -04:00
Ken Brown 71726ba70b Cygwin: FIFO: use a cygthread instead of a homemade thread
This will simplify future work.

Rename the thread from "listen_client_thread" to "fifo_reader_thread"
because it will be used for more than just listening.

Remove the fixup_before stuff, which won't be needed after future
changes to fixup_after_fork and fixup_after_exec.
2020-05-08 06:32:00 -04:00
Ken Brown 9ee8fdf2b3 Cygwin: FIFO: make opening a writer more robust
- Make read_ready a manual-reset event.

- Signal read_ready in open instead of in the listen_client_thread.

- Don't reset read_ready when the listen_client thread terminates;
  instead do it in close().

- Rearrange open and change its error handling.

- Add a wait_open_pipe method that waits for a pipe instance to be
  available and then calls open_pipe.  Use it when opening a writer if
  we can't connect immediately.  This can happen if the system is
  heavily loaded and/or if many writers are trying to open
  simultaneously.
2020-05-08 06:32:00 -04:00
Ken Brown 301454f132 Cygwin: FIFO: fix hit_eof
According to Posix, a FIFO open for reading is at EOF if it is empty
and there are no writers open.

The only way to test this is to poll the fifo_client_handlers as in
raw_read and select.cc:peek_fifo.  The current hit_eof instead relies
on the value of nconnected, which can be out of date.  On the one
hand, it doesn't take into account writers that were connected but
have since closed.  On the other hand, it doesn't take into account
writers that are in the process of opening but haven't yet connected.

Fix this by introducing a maybe_eof method that tentatively assumes
EOF if there are no connected writers after polling.  Then check for
writers currently opening (via a new 'writer_opening' event), and wait
for the fifo_reader_thread to record any new connection that was made
while we were polling.

To handle the needs of peek_fifo, replace the get_fc_handle method
by a get_fc_handler method, and add a fifo_client_handler::get_state
method.

Remove the is_connected method, which was used only in peek_fifo and
is no longer needed.

Remove the nconnected data member, which was used only for the flawed
hit_eof.

Add some comments about events to fhandler.h.
2020-05-08 06:32:00 -04:00
Ken Brown 13c65c43c2 Cygwin: FIFO: dup/fork/exec: make sure child starts unlocked
There can be deadlocks if the child starts with its fifo_client_lock
in the locked state.
2020-05-08 06:32:00 -04:00
Ken Brown 624fda1e96 Cygwin: FIFO: honor the flags argument in dup
Also improve the error handling.
2020-05-08 06:32:00 -04:00
Ken Brown 25e8727368 Cygwin: FIFO: remove the arm method
There's no reason to check for errors when we set read_ready or
write_ready.  We don't do that for other events.
2020-05-08 06:32:00 -04:00
Ken Brown 9b2afd78ce Cygwin: FIFO: simplify the listen_client_thread code
Always return 0; no one is doing anything with the return value
anyway.

Remove the return value from stop_listen_client.

Make the connection event auto-reset, so that we don't have to reset
it later.

Simplify the process of connecting a bogus client when thread
termination is signaled.

Make some failures fatal.

Remove the unnecessary extra check for thread termination near the end
of listen_client_thread.
2020-05-08 06:32:00 -04:00
Ken Brown 32dbc3d215 Cygwin: FIFO: change the fifo_client_connect_state enum
Make the values correspond to the possible return values of
fifo_client_handler::pipe_state().

When cleaning up the fc_handler list in listen_client_thread(), don't
delete handlers in the fc_closing state.  I think the pipe might still
have input to be read in that case.

Set the state to fc_closing later in the same function if a connection
is made and the status returned by NtFsControlFile is
STATUS_PIPE_CLOSING.

In raw_read, don't error out if NtReadFile returns an unexpected
status; just set the state of that handler to fc_error.  One writer in
a bad state doesn't justify giving up on reading.
2020-05-08 06:32:00 -04:00
Ken Brown ce23e97640 Cygwin: FIFO: simplify the fifo_client_handler structure
Replace the 'fhandler_base *' member by a HANDLE to the server side of
the Windows named pipe instance.  Make the corresponding
simplifications throughout.
2020-05-08 06:32:00 -04:00
Ken Brown d05124dc6b Cygwin: FIFO: minor change - use NtClose
Replace CloseHandle by NtClose since all handles are created by NT
functions.
2020-05-08 06:32:00 -04:00
Ken Brown 279f230620 Cygwin: fhandler_fifo.cc: add commentary 2020-02-01 16:36:31 -05:00
Ken Brown 13d75fce48 Cygwin: FIFO: fstatvfs: use our handle if O_PATH is set
If O_PATH is set, then the fhandler_fifo object has a handle that can
be used for getting the statvfs information.  Use it by calling
fhandler_base::fstatvfs_by_handle.  Before this change,
fhandler_disk_file::fstatfvs was called on a new fhandler_disk_file
object, which would then have to be opened.
2020-01-28 14:05:13 -05:00
Ken Brown 7d68ffadd3 Cygwin: FIFO: tweak fcntl and dup when O_PATH is set
fhandler_fifo::fcntl and fhandler_fifo::dup now call the corresponding
fhandler_base methods if the FIFO was opened with the O_PATH flag.
2020-01-24 11:24:18 +01:00
Ken Brown 5fb3790422 Cygwin: re-implement fhandler_fifo::open with O_PATH
If the O_PATH flag is set, fhandler_fifo::open now simply calls
fhandler_base::open_fs.

The previous attempt to handle O_PATH in commit aa55d22c, "Cygwin:
honor the O_PATH flag when opening a FIFO", fixed a hang but otherwise
didn't do anything useful.
2020-01-24 11:24:18 +01:00
Ken Brown 3bc79b275e Cygwin: FIFO: use FILE_PIPE_REJECT_REMOTE_CLIENTS flag
Add that flag to the pipe type argument when creating the Windows
named pipe.  And add a definition of that flag to ntdll.h (copied from
/usr/include/w32api/ddk/ntifs.h).
2019-12-21 17:53:52 -05:00
Ken Brown aa55d22cb5 Cygwin: honor the O_PATH flag when opening a FIFO
Previously fhandler_fifo::open would treat the FIFO as a reader and
would block, waiting for a writer.
2019-06-27 07:46:14 -04:00
Ken Brown 09e2ec87ef Cygwin: FIFO: fix a thinko in raw_write
Remove a line that has no effect.
2019-06-25 18:45:34 -04:00
Ken Brown d54edfdf81 Cygwin: FIFO: minor cleanup
Don't use a label with the same name as a variable.

Also fix indentation in fhandler.h.
2019-06-23 10:16:34 -04:00