Switch Win32 pipes to PIPE_WAIT

[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 more fundamental way, but slightly changes the requirements of libdispatch: #854.

• 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.

#796 attempts to bound the max size of the write to avoid blocking the dispatch thread:

swift-corelibs-libdispatch/src/io.c

Lines 2516 to 2528 in 0c38954

	IO_STATUS_BLOCK iosb;
	FILE_PIPE_LOCAL_INFORMATION fpli;
	NTSTATUS status = _dispatch_NtQueryInformationFile(hFile,
	&iosb, &fpli, sizeof(fpli), FilePipeLocalInformation);
	if (NT_SUCCESS(status)) {
	// WriteQuotaAvailable is unreliable in the presence
	// of a blocking reader, when it can return zero, so only
	// account for it otherwise
	if (fpli.WriteQuotaAvailable > 0) {
	len = MIN(len, fpli.WriteQuotaAvailable);
	}
	len = MIN(len, fpli.OutboundQuota);
	}

• We first check the WriteQuotaAvailable of the pipe (which is the size of the pipe's buffer - the total amount of space queued to be read - the bytes already present in the output buffer), and use this as an initial upper bound.
• If WriteQuotaAvailable == 0, this implies we either have a reader that has requested a full buffer's worth of data from the pipe, and so we bound by the size of the pipe's buffer (OutboundQuota) OR the output buffer is already full.
• In the first case, the write will not block because the data fits into the pipe's outbound buffer.
• In the second case (the output buffer is already full), the write will block until data is drained from the pipe by a reader.

So if nothing is actively reading the data, the queue will be blocked from making progress. However, I have been unable to trigger this case in practice. If there is no reader on the pipe, WriteFile will fail quickly. From what I could grok from the libdispatch code, the pipe synchronization mechanism will always have a reader waiting on some I/O Completion Port that will quickly resume when data in the pipe is available to be read.

I am reasonably confident in this conclusion, but because I was never able to reproduce the original hanging write with the write-bounding logic removed, I have been unable to verify that the write hang issue cannot occur in practice with this PR. I tested this under many different scenarios, both realistic and pathological, but never encountered this particular issue.

With this PR though, I can verify that the spinning thread is gone, and programs which rely on libdispatch exhibit considerably better CPU performance (namely sourcekit-lsp).

cc @compnerd

[tristanlabelle]

Since PIPE_WAIT is the default, I think we should only assert if it is not set rather than changing the state. We changed the state to NOWAIT when we thought we needed it because it was not the default. If callers set NOWAIT now, we can consider ill effects to be their fault.

[z2oh]

If callers set NOWAIT now, we can consider ill effects to be their fault.

Why wouldn't we try to "fix" the pipe in that case? If my understanding is correct that ownership of the pipe is transferred to libdispatch, it makes sense to me that we should try to make it conform to the expected semantics.

[tristanlabelle]

_dispatch_pipe_monitor_thread is the reader and this code is for the writer. We shouldn't assume that the reader thread is using libdispatch. We could just assert that it's not NOWAIT and comment that the writer is misbehaved if NOWAIT is set, regardless of what the reader does.

[z2oh]

Hmm, I'm not following. _dispatch_pipe_monitor_thread isn't actually the reader, it just waits until data is available on the pipe and then signals to the real reader that data is available. I don't think you can get around using _dispatch_pipe_monitor_thread if you are writing data through a Win32 pipe on libdispatch, so I think the comment is relevant here.