Skip to content

Switch Win32 pipes to PIPE_WAIT #853

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Switch Win32 pipes to PIPE_WAIT #853

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/event/event_windows.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,9 @@ _dispatch_pipe_monitor_thread(void *context)
char cBuffer[1];
DWORD dwNumberOfBytesTransferred;
OVERLAPPED ov = {0};
// Block on a 0-byte read; this will only resume when data is
// available in the pipe. The pipe must be PIPE_WAIT or this thread
// will spin.
BOOL bSuccess = ReadFile(hPipe, cBuffer, /* nNumberOfBytesToRead */ 0,
&dwNumberOfBytesTransferred, &ov);
DWORD dwBytesAvailable;
Expand Down
20 changes: 10 additions & 10 deletions src/io.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1437,20 +1437,20 @@ _dispatch_fd_entry_create_with_fd(dispatch_fd_t fd, uintptr_t hash)
int result = ioctlsocket((SOCKET)fd, (long)FIONBIO, &value);
(void)dispatch_assume_zero(result);
} else {
// Try to make writing nonblocking, although pipes not coming
// from Foundation.Pipe may not have FILE_WRITE_ATTRIBUTES.
// The _dispatch_pipe_monitor_thread expects pipes to be
// PIPE_WAIT and exploits this assumption by using a blocking
// 0-byte read as a synchronization mechanism.
Comment on lines +1440 to +1442
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

DWORD dwPipeMode = 0;
if (GetNamedPipeHandleState((HANDLE)fd, &dwPipeMode, NULL,
NULL, NULL, NULL, 0) && !(dwPipeMode & PIPE_NOWAIT)) {
dwPipeMode |= PIPE_NOWAIT;
NULL, NULL, NULL, 0) && !(dwPipeMode & PIPE_WAIT)) {
dwPipeMode |= PIPE_WAIT;
if (!SetNamedPipeHandleState((HANDLE)fd, &dwPipeMode,
NULL, NULL)) {
Comment on lines 1444 to 1448
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// We may end up blocking on subsequent writes, but we
// don't have a good alternative.
// The WriteQuotaAvailable from NtQueryInformationFile
// erroneously returns 0 when there is a blocking read
// on the other end of the pipe.
_dispatch_fd_entry_debug("failed to set PIPE_NOWAIT",
// If setting the pipe to PIPE_WAIT fails, the
// monitoring thread will spin constantly, saturating
// a core, which is undesirable but non-fatal.
// The semantics will still be correct in this case.
_dispatch_fd_entry_debug("failed to set PIPE_WAIT",
fd_entry);
}
}
Expand Down