Switch Win32 pipes to PIPE_WAIT with sentinel bufsize

[z2oh]

Fixes #820 . There is a lot of useful context in this issue, which I will partially reproduce below.

I have an alternate PR that fixes this problem in a less fundamental way, but without a small requirements change to libdispatch which I describe below: #853.

Problem description, copied from the aforementioned PR:

• In Use PIPE_NOWAIT on Windows to avoid blocking on writes #781, the pipe was changed from PIPE_WAIT to PIPE_NOWAIT to achieve POSIX-like O_NONBLOCK semantics, as attempting to write a large buffer would cause the dispatch queue thread to block long enough to hit a time out.
• The Windows pipe synchronization thread, _dispatch_pipe_monitor_thread, uses a blocking 0-byte read as a synchronization mechanism to signal to the actual reader threads that there is data waiting in the pipe. Of course, for this to work, the read must actually be blocking. With a PIPE_NOWAIT pipe, the Read will not block, causing the thread to spin endlessly and constantly wake the actual reader thread to perform 0 byte reads. This spinning thread is the root source of the problem in this issue.

Switching the pipe back to PIPE_WAIT potentially resurfaces the blocking writes problem, however I think that #796 (which was a follow-up PR to fix a problem with the PIPE_NOWAIT implementation) also resolves the blocking write problem in practice but not necessarily in all cases.

The real issue here is the inability to distinguish on the write side of a pipe between the following cases:

• The pipe's output buffer is full (we do not want to write as calling WriteFile will block)
• The read side of the pipe has requested a full buffer's worth of data (we do want to write because we need to make progress and there is a reader waiting for it)

This PR implements the same switch back to PIPE_WAIT as in #853, but uses a trick to enable WriteQuotaAvailable == 1 to be used as a sentinel value to distinguish the "output buffer is full" case. By always leaving a free byte of space in the buffer when writing, the only way that WriteQuotaAvailable == 0 can happen is if a reader has requested a full buffer's worth of data.

This means that when we see WriteQuotaAvailable == 1, we know the output buffer is "full" and not attempt to perform a blocking write.

The downside to this approach is that we can no longer serially write-then-read a full buffer's worth of data, because the write will not finish writing the last byte until reading has commenced. I don't see this scenario ever arising in practice: if you are trying to move data serially from one buffer to another on the same thread, there are better ways to do that than through a libdispatch pipe. But regardless, this requirement is implicit due to the serial nature of the dispatch_io_pipe tests. If if is acceptable to relax this requirement from "writes of <=bufSize are guaranteed to complete in the absence of a reader" to "writes of <bufSize are guaranteed to complete in the absence of a reader" then I think this solution is more robust than just #853.

It's entirely possible I'm missing some good reason why this requirement is there though, in which case I think #853 should be acceptable.

cc @compnerd

[tristanlabelle]

I'm comfortable with this fix

[maruel]

Thanks for looking at this! Just a comment, no opinion on the change.

[maruel]

Why not do a WaitForSingleObjectEx()? In particular pay attention to the bAlertable condition.

IMHO, I don't see the need to use a thread, WaitForMultipleObjectsEx() would do just fine and be more performant.

Note that I don't have a good understanding of this code base so my humble opinion may be wrong but I have some experience with win32 pipes.

[z2oh]

I'm not sure I follow. I looked through the docs of WaitForSingleObjectEx and I don't see where it applies here. You can't wait on a pipe, no?

[maruel]

Effectively. The idea is to wait on the events that are connected to the pending overlappedio.

[maruel]

Expanding; https://learn.microsoft.com/en-us/windows/win32/devio/overlapped-operations

The OVERLAPPED structure must contain a handle to a manual-reset (not an auto-reset) event object

https://learn.microsoft.com/en-us/windows/win32/api/minwinbase/ns-minwinbase-overlapped
especially the section about hEvent.

[maruel]

Or said differently, I didn't recall passing an OVERLAPPED structure with a null hEvent to be valid.

[z2oh]

In general we cannot rely on Overlapped I/O here (which would simplify many things). Pipes are created externally and their handles are passed into libdispatch, and there's no way to make a pipe FILE_FLAG_OVERLAPPED after the fact (at least, none that I could find, and I looked for this).

We also need to support anonymous pipes, which do not support Overlapped I/O. This requirement is embedded in the libdispatch tests, which use anonymous pipes.

It could be I'm misunderstanding something about your proposal though. After discounting Overlapped I/O because the requirements mentioned above, I stopped looking into that solution space, so am less familiar with the APIs you are referencing.

[maruel]

Oh ok that's a bummer. Sorry I didn't mean to send you off track. From https://learn.microsoft.com/en-us/windows/win32/ipc/anonymous-pipe-operations:

In addition, the lpOverlapped parameter of ReadFile and WriteFile is ignored when these functions are used with anonymous pipes.

Emphasis mine. That's why the code works. I'd recommend to pass NULL instead or &ov to avoid confusion. Other than that, lgtm.