Skip to content
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.

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

Support asynchronous IO.pipe on Windows #13362

Merged

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Apr 20, 2023

This PR makes IO.pipe return IO::FileDescriptors whose #read and #write run asynchronously. Closes #6957. Fixes #12718. Fixes #9893 (comment).

IO.pipe is backed by Win32 named pipes with unique names. They differ from Unix pipes in at least two substantial ways:

  • One end is the pipe server and the opposite end is the pipe client, even when both ends are only used in the same process. The pipe direction is independent of the server-client relationship; this PR makes the write end the server end.
  • Closing the write end does not signal an EOF on the read end, but it does trigger ERROR_BROKEN_PIPE, so that error value is used for this purpose instead and not treated as a hard error.

Asynchronous I/O for regular files might be as simple as passing LibC::FILE_FLAG_OVERLAPPED to LibC.CreateFileW, which I haven't tried. (They will signal EOFs.) The console STDIN and STDOUT are also still synchronous and might require separate threads as described in #6957 (comment).

Also it seems this is not enough to get the playground running on Windows.

@HertzDevil HertzDevil marked this pull request as ready for review April 20, 2023 13:52
@HertzDevil HertzDevil added the platform:windows Windows support based on the MSVC toolchain / Win32 API label Apr 20, 2023
Comment on lines 82 to 84
{% unless flag?(:win32) %}
server_io.close
if exc = done.receive
raise exc
end
{% end %}
Copy link
Contributor Author

@HertzDevil HertzDevil Apr 20, 2023

Choose a reason for hiding this comment

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

It seems one of the IOs was trying to flush to a closed stream if this line is present. I have no idea this broke the OAuth2 specs nor why it is necessary on existing platforms

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this should require some investigation. The close behaviour should not be platform-specific.

Copy link
Contributor Author

@HertzDevil HertzDevil Apr 25, 2023

Choose a reason for hiding this comment

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

It looks like the current fiber is unconditionally scheduled at a later time even if WriteFile finishes synchronously, despite FILE_FLAG_OVERLAPPED being used. This causes this ensure block to run immediately when the HTTP::Server::Response is closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I'm wondering: if socket operations like WSASend complete immediately, we also might not need to unconditionally reschedule inside #wsa_overlapped_operation either

Copy link
Member

Choose a reason for hiding this comment

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

Great find! That seems like the piece we've been missing 🚀

@straight-shoota
Copy link
Member

straight-shoota commented Apr 21, 2023

My main worry is the behaviour of async (overlapped) streams being passed to a new process. As described in #6957 (comment) point 3. this would be problematic.
This issues does not appear with most typical process IO. The pipes created by Process#stdio_to_fd are blocking on the end that goes to the new process.
But when you explicitly pass a file descriptor that's non-blocking (such as one created by IO.pipe), there may be problems.

For example, the following program has lead to crashes with this patch:

reader, writer = IO.pipe
Process.run("cmd.exe", output: writer)
writer.close
puts reader.gets_to_end

It doesn't always crash. And I haven't been able to dig more into what's going on. When it's crashing it seems to take the shell down with it. And nothing is written to stdout nor stderr. So this might need more investigation.

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Apr 21, 2023

I tried the above on Windows 11 22H2 22621.1555 and it never crashed.

Can Process.new simply insert another named pipe in-between if it finds that an IO is non-blocking? We don't have to unconditionally pass any IO::FileDescriptor unmodified to Crystal::System::Process.spawn.

@straight-shoota
Copy link
Member

straight-shoota commented Apr 21, 2023

Hm, after more digging it seems that the compiler (or the wrapper script?) is actually breaking. Not the program itself (I was using run and eval so it wasn't clear what's happening). I still don't know what's going on there but it seems unrelated. I managed to run the program consistently successful when building it a different way. So we're probably good on this.

@kubo Do you have any comment? Maybe we need to consider something we have missed so far?

@kubo
Copy link
Contributor

kubo commented Apr 23, 2023

@kubo Do you have any comment? Maybe we need to consider something we have missed so far?

I checked it using this gist code. As far as I ran the code, asynchronous pipe handles can be passed to child processes as stdio handles. fgets and fputs work fine in the child processes. However asynchronous file handles cannot. fgets fails with errno 22 (Invalid argument). fputs doesn't return error but writes nothing.

As for strikethrough text in #6957 (comment) it seems that I made a mistake. Perhaps it was fixed by MS or probably I wrote wrong code.

@kubo kubo mentioned this pull request Apr 23, 2023
4 tasks
src/crystal/system/win32/file_descriptor.cr Outdated Show resolved Hide resolved
Comment on lines 82 to 84
{% unless flag?(:win32) %}
server_io.close
if exc = done.receive
raise exc
end
{% end %}
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this should require some investigation. The close behaviour should not be platform-specific.

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

LGTM

@straight-shoota straight-shoota added this to the 1.9.0 milestone Apr 27, 2023
@straight-shoota straight-shoota merged commit dfa8fe4 into crystal-lang:master Apr 28, 2023
@HertzDevil HertzDevil deleted the feature/windows-async-io-pipe branch April 28, 2023 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature platform:windows Windows support based on the MSVC toolchain / Win32 API topic:stdlib:concurrency topic:stdlib:files
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Process.run hangs on windows in conditions Port concurrency features to Windows
3 participants