From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2210) id CB6A9385041D; Tue, 4 Aug 2020 15:29:41 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org CB6A9385041D Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: Ken Brown To: cygwin-cvs@sourceware.org Subject: [newlib-cygwin] Cygwin: FIFO: synchronize the fifo_reader and fifosel threads X-Act-Checkin: newlib-cygwin X-Git-Author: Ken Brown X-Git-Refname: refs/heads/master X-Git-Oldrev: 251624a3529c3d0d3d9cdb5fbaf4c3e9b079c894 X-Git-Newrev: 0fda55133a82eb70e98e636d0d00f48c674b9440 Message-Id: <20200804152941.CB6A9385041D@sourceware.org> Date: Tue, 4 Aug 2020 15:29:41 +0000 (GMT) X-BeenThere: cygwin-cvs@cygwin.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Cygwin core component git logs List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 04 Aug 2020 15:29:41 -0000 https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=0fda55133a82eb70e98e636d0d00f48c674b9440 commit 0fda55133a82eb70e98e636d0d00f48c674b9440 Author: Ken Brown Date: Mon Aug 3 09:38:08 2020 -0400 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. Diff: --- 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 ());