From 9ee8fdf2b3a371cb14fb902a8c9aab76dd60f4e2 Mon Sep 17 00:00:00 2001 From: Ken Brown Date: Tue, 17 Mar 2020 14:14:47 -0400 Subject: [PATCH] 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. --- winsup/cygwin/fhandler.h | 1 + winsup/cygwin/fhandler_fifo.cc | 271 ++++++++++++++++++++------------- 2 files changed, 170 insertions(+), 102 deletions(-) diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h index 3bc04cf13..2516c93b4 100644 --- a/winsup/cygwin/fhandler.h +++ b/winsup/cygwin/fhandler.h @@ -1323,6 +1323,7 @@ class fhandler_fifo: public fhandler_base static NTSTATUS npfs_handle (HANDLE &); HANDLE create_pipe_instance (bool); NTSTATUS open_pipe (HANDLE&); + NTSTATUS wait_open_pipe (HANDLE&); int add_client_handler (); void delete_client_handler (int); bool listen_client (); diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc index 21faf4ec2..5c3df5497 100644 --- a/winsup/cygwin/fhandler_fifo.cc +++ b/winsup/cygwin/fhandler_fifo.cc @@ -222,7 +222,64 @@ fhandler_fifo::open_pipe (HANDLE& ph) openflags & O_CLOEXEC ? 0 : OBJ_INHERIT, npfsh, NULL); sharing = FILE_SHARE_READ | FILE_SHARE_WRITE; - status = NtOpenFile (&ph, access, &attr, &io, sharing, 0); + return NtOpenFile (&ph, access, &attr, &io, sharing, 0); +} + +/* Wait up to 100ms for a pipe instance to be available, then connect. */ +NTSTATUS +fhandler_fifo::wait_open_pipe (HANDLE& ph) +{ + HANDLE npfsh; + HANDLE evt; + NTSTATUS status; + IO_STATUS_BLOCK io; + ULONG pwbuf_size; + PFILE_PIPE_WAIT_FOR_BUFFER pwbuf; + LONGLONG stamp; + LONGLONG orig_timeout = -100 * NS100PERSEC / MSPERSEC; /* 100ms */ + + status = npfs_handle (npfsh); + if (!NT_SUCCESS (status)) + return status; + if (!(evt = create_event ())) + api_fatal ("Can't create event, %E"); + pwbuf_size + = offsetof (FILE_PIPE_WAIT_FOR_BUFFER, Name) + get_pipe_name ()->Length; + pwbuf = (PFILE_PIPE_WAIT_FOR_BUFFER) alloca (pwbuf_size); + pwbuf->Timeout.QuadPart = orig_timeout; + pwbuf->NameLength = get_pipe_name ()->Length; + pwbuf->TimeoutSpecified = TRUE; + memcpy (pwbuf->Name, get_pipe_name ()->Buffer, get_pipe_name ()->Length); + stamp = get_clock (CLOCK_MONOTONIC)->n100secs (); + bool retry; + do + { + retry = false; + status = NtFsControlFile (npfsh, evt, NULL, NULL, &io, FSCTL_PIPE_WAIT, + pwbuf, pwbuf_size, NULL, 0); + if (status == STATUS_PENDING) + { + if (WaitForSingleObject (evt, INFINITE) == WAIT_OBJECT_0) + status = io.Status; + else + api_fatal ("WFSO failed, %E"); + } + if (NT_SUCCESS (status)) + status = open_pipe (ph); + if (STATUS_PIPE_NO_INSTANCE_AVAILABLE (status)) + { + /* Another writer has grabbed the pipe instance. Adjust + the timeout and keep waiting if there's time left. */ + pwbuf->Timeout.QuadPart = orig_timeout + + get_clock (CLOCK_MONOTONIC)->n100secs () - stamp; + if (pwbuf->Timeout.QuadPart < 0) + retry = true; + else + status = STATUS_IO_TIMEOUT; + } + } + while (retry); + NtClose (evt); return status; } @@ -294,7 +351,6 @@ void fhandler_fifo::record_connection (fifo_client_handler& fc, fifo_client_connect_state s) { - SetEvent (write_ready); fc.state = s; maybe_eof (false); ResetEvent (writer_opening); @@ -327,9 +383,6 @@ fhandler_fifo::listen_client_thread () if (add_client_handler () < 0) api_fatal ("Can't add a client handler, %E"); - /* Allow a writer to open. */ - SetEvent (read_ready); - /* Listen for a writer to connect to the new client handler. */ fifo_client_handler& fc = fc_handler[nhandlers - 1]; NTSTATUS status; @@ -405,19 +458,13 @@ fhandler_fifo::listen_client_thread () out: if (conn_evt) NtClose (conn_evt); - ResetEvent (read_ready); return 0; } int fhandler_fifo::open (int flags, mode_t) { - enum - { - success, - error_errno_set, - error_set_errno - } res; + int saved_errno = 0; if (flags & O_PATH) return open_fs (flags); @@ -437,8 +484,7 @@ fhandler_fifo::open (int flags, mode_t) break; default: set_errno (EINVAL); - res = error_errno_set; - goto out; + goto err; } debug_only_printf ("reader %d, writer %d, duplexer %d", reader, writer, duplexer); @@ -454,135 +500,151 @@ fhandler_fifo::open (int flags, mode_t) char npbuf[MAX_PATH]; __small_sprintf (npbuf, "r-event.%08x.%016X", get_dev (), get_ino ()); - if (!(read_ready = CreateEvent (sa_buf, false, false, npbuf))) + if (!(read_ready = CreateEvent (sa_buf, true, false, npbuf))) { debug_printf ("CreateEvent for %s failed, %E", npbuf); - res = error_set_errno; - goto out; + __seterrno (); + goto err; } npbuf[0] = 'w'; if (!(write_ready = CreateEvent (sa_buf, true, false, npbuf))) { debug_printf ("CreateEvent for %s failed, %E", npbuf); - res = error_set_errno; - goto out; + __seterrno (); + goto err_close_read_ready; } npbuf[0] = 'o'; if (!(writer_opening = CreateEvent (sa_buf, true, false, npbuf))) { debug_printf ("CreateEvent for %s failed, %E", npbuf); - res = error_set_errno; - goto out; + __seterrno (); + goto err_close_write_ready; } - /* If we're a duplexer, create the pipe and the first client handler. */ - if (duplexer) - { - HANDLE ph = NULL; - - if (add_client_handler () < 0) - { - res = error_errno_set; - goto out; - } - NTSTATUS status = open_pipe (ph); - if (NT_SUCCESS (status)) - { - record_connection (fc_handler[0]); - set_handle (ph); - set_pipe_non_blocking (ph, flags & O_NONBLOCK); - } - else - { - __seterrno_from_nt_status (status); - res = error_errno_set; - goto out; - } - } - - /* If we're reading, start the listen_client thread (which should - signal read_ready), and wait for a writer. */ + /* If we're reading, signal read_ready and start the listen_client + thread. */ if (reader) { if (!listen_client ()) { debug_printf ("create of listen_client thread failed"); - res = error_errno_set; - goto out; + goto err_close_writer_opening; } - else if (!duplexer && !wait (write_ready)) + SetEvent (read_ready); + + /* If we're a duplexer, we need a handle for writing. */ + if (duplexer) { - res = error_errno_set; - goto out; - } - else - { - init_fixup_before (); - res = success; + HANDLE ph = NULL; + NTSTATUS status; + + while (1) + { + status = open_pipe (ph); + if (NT_SUCCESS (status)) + { + set_handle (ph); + set_pipe_non_blocking (ph, flags & O_NONBLOCK); + break; + } + else if (status == STATUS_OBJECT_NAME_NOT_FOUND) + { + /* The pipe hasn't been created yet. */ + yield (); + continue; + } + else + { + __seterrno_from_nt_status (status); + goto err_close_reader; + } + } } + /* Not a duplexer; wait for a writer to connect. */ + else if (!wait (write_ready)) + goto err_close_reader; + init_fixup_before (); + goto success; } - /* If we're writing, wait for read_ready and then connect to the - pipe. This should always succeed quickly if the reader's - listen_client thread is running. Then signal write_ready. */ + /* If we're writing, wait for read_ready, connect to the pipe, and + signal write_ready. */ if (writer) { + NTSTATUS status; + SetEvent (writer_opening); + if (!wait (read_ready)) + { + ResetEvent (writer_opening); + goto err_close_writer_opening; + } while (1) { - if (!wait (read_ready)) - { - ResetEvent (writer_opening); - res = error_errno_set; - goto out; - } - NTSTATUS status = open_pipe (get_handle ()); + status = open_pipe (get_handle ()); if (NT_SUCCESS (status)) + goto writer_success; + else if (status == STATUS_OBJECT_NAME_NOT_FOUND) { - set_pipe_non_blocking (get_handle (), flags & O_NONBLOCK); - SetEvent (write_ready); - res = success; - goto out; + /* The pipe hasn't been created yet. */ + yield (); + continue; } else if (STATUS_PIPE_NO_INSTANCE_AVAILABLE (status)) - Sleep (1); + break; else { debug_printf ("create of writer failed"); __seterrno_from_nt_status (status); - res = error_errno_set; ResetEvent (writer_opening); - goto out; + goto err_close_writer_opening; + } + } + + /* We should get here only if the system is heavily loaded + and/or many writers are trying to connect simultaneously */ + while (1) + { + SetEvent (writer_opening); + if (!wait (read_ready)) + { + ResetEvent (writer_opening); + goto err_close_writer_opening; + } + status = wait_open_pipe (get_handle ()); + if (NT_SUCCESS (status)) + goto writer_success; + else if (status == STATUS_IO_TIMEOUT) + continue; + else + { + debug_printf ("create of writer failed"); + __seterrno_from_nt_status (status); + ResetEvent (writer_opening); + goto err_close_writer_opening; } } } -out: - if (res == error_set_errno) - __seterrno (); - if (res != success) - { - if (read_ready) - { - NtClose (read_ready); - read_ready = NULL; - } - if (write_ready) - { - NtClose (write_ready); - write_ready = NULL; - } - if (writer_opening) - { - NtClose (writer_opening); - writer_opening = NULL; - } - if (get_handle ()) - NtClose (get_handle ()); - if (listen_client_thr) - stop_listen_client (); - } - debug_printf ("res %d", res); - return res == success; +writer_success: + set_pipe_non_blocking (get_handle (), flags & O_NONBLOCK); + SetEvent (write_ready); +success: + return 1; +err_close_reader: + saved_errno = get_errno (); + close (); + set_errno (saved_errno); + return 0; +err_close_writer_opening: + NtClose (writer_opening); +err_close_write_ready: + NtClose (write_ready); +err_close_read_ready: + NtClose (read_ready); +err: + if (get_handle ()) + NtClose (get_handle ()); + return 0; } off_t @@ -938,6 +1000,11 @@ fhandler_fifo::close () handler or another thread. */ fifo_client_unlock (); stop_listen_client (); + if (reader) + /* FIXME: There could be several readers open because of + dup/fork/exec; we should only reset read_ready when the last + one closes. */ + ResetEvent (read_ready); if (read_ready) NtClose (read_ready); if (write_ready)