On Tue, 14 Sep 2021 04:37:18 +0900 Takashi Yano wrote: > On Mon, 13 Sep 2021 20:32:33 +0200 > Corinna Vinschen wrote: > > On Sep 12 20:04, Takashi Yano wrote: > > > On Sun, 12 Sep 2021 17:48:49 +0900 > > > Takashi Yano wrote: > > > > Hmm. Then, what about PoC code attached? This returns to Corinna's > > > > query_hdl, and counts read/write handles to detect closing reader side. > > > > > > > > If the number of read handles is equal to number of write handles, > > > > only the pairs of write handle and query_hdl are alive. So, read pipe > > > > supposed to be closed. > > > > > > > > This patch depends another patch I posted a few hours ago. > > > > > > Revised a bit. > > > [...] > > > > What I miss is a bit more detailed commit message... > > I am sorry, I will add more detail commit message. Done. > > > diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc > > > index 9b4255cfd..b051f5c03 100644 > > > --- a/winsup/cygwin/fhandler_pipe.cc > > > +++ b/winsup/cygwin/fhandler_pipe.cc > > > @@ -56,6 +56,8 @@ fhandler_pipe::set_pipe_non_blocking (bool nonblocking) > > > fpi.ReadMode = FILE_PIPE_BYTE_STREAM_MODE; > > > fpi.CompletionMode = nonblocking ? FILE_PIPE_COMPLETE_OPERATION > > > : FILE_PIPE_QUEUE_OPERATION; > > > + if (query_hdl) > > > + fpi.CompletionMode = FILE_PIPE_COMPLETE_OPERATION; > > > > This should be a single expression, i.e. > > > > fpi.CompletionMode = nonblocking || query_hdl > > ? FILE_PIPE_COMPLETE_OPERATION > > : FILE_PIPE_QUEUE_OPERATION; > > > > ideally combined with a comment. > > Thanks. I'll do that. Done. > > But then again... you're basically switching the write side of > > a pipe to nonblocking mode unconditionally. The downside is a > > busy wait: > > > > > fhandler_pipe_fifo::raw_write (const void *ptr, size_t len) > > > { > > > @@ -493,7 +490,20 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t len) > > > get_obj_handle_count (select_sem), NULL); > > > /* 0 bytes returned? EAGAIN. See above. */ > > > if (NT_SUCCESS (status) && nbytes == 0) > > > - set_errno (EAGAIN); > > > + { > > > + if (reader_closed ()) > > > + { > > > + set_errno (EPIPE); > > > + raise (SIGPIPE); > > > + } > > > + else if (is_nonblocking ()) > > > + set_errno (EAGAIN); > > > + else > > > + { > > > + cygwait (select_sem, 10); > > > + continue; > > > > I'm a bit puzzled. The cygwait branch neglects to check if select_sem > > is NULL (the preceeding ReleaseSemaphore expression does!) > > And then it doesn't matter if the caller got blocked or not, it will > > always perform a continue. So why do it at all? Worse, if this > > expression loops, it will eat up the semaphore, because each call will > > decrement the semaphore count until it blocks. That sounds wrong to me. > > It is by design. ReleaseSemaphore() releases maximum number of semaphore > which the waiter can exists. If only one writer and one reader exist, > ReleaseSemaphore releases 2 semaphores. Then cygwait here consume semaphore > two times and return to wait state. > This wait state is released by raw_read() or close(). > > > Btw., while looking into the current pipe code, I wonder what select_sem > > is doing in the pipe code at all so far. It gets released, but it never > > gets waited on?!? Am I missing something? > > The semaphore is waited in select.cc. > But, wait. Wat happens if select() is not called? Released semaphore > can be accumulated up to INT32_MAX!!? > > Let me consider. See the second patch attached. With this patch, only minimum number of semaphores needed are released. Please apply following two patches I attached to previous mail first: 0001-Cygwin-fhandler_base-dup-Reflect-O_CLOEXEC-to-inheri.patch 0002-Cygwin-pipe-fifo-Call-set_no_inheritance-for-adjunct.patch Then, apply the patches attached this mail. 0001-Cygwin-pipe-Use-read-pipe-handle-for-select-on-write.patch 0002-Cygwin-pipe-fifo-Release-select_sem-semaphore-as-muc.patch Thanks -- Takashi Yano