-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
net: Provide a raw NamedPipe implementation and builders #3760
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general this seems like a good start. It's very close to the underlying raw API, which is what I would want. Abstractions can be built on top later or elsewhere.
pub async fn connect(&self) -> io::Result<()> { | ||
loop { | ||
match self.io.connect() { | ||
Ok(()) => break, | ||
Err(e) if e.kind() == io::ErrorKind::WouldBlock => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So with the current API, this means that the type system doesn't prevent you from calling this on a client socket, or on a connected socket. This might be ok, but it is worth to discuss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After giving the available APIs a more thorough look through I am loosening up to the idea of having NamedPipeClient
and NamedPipeServer
. There aren't that many common APIs, and the ones that exist (e.g. GetNamedPipeInfo
, TransactNamedPipe
, PeekNamedPipe
, ...) could be duplicated if we find it necessary to introduce them. Still, the idea of only having one makes the API more obvious to design - but leaves it up to the user to use correctly.
It is worth noting that anonymous named pipes (as the ones created through CreatePipe
https://docs.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-createpipe) would probably require their own wrapper, or maybe just use NamedPipeClient
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The miow
crate uses a separate type for the anonymous pipes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So after a bit of work I've come to the conclusion that CreatePipe
et. al. does not appear to be suitable for constructing handles that can be used in Tokio. There's no way to set FILE_FLAG_OVERLAPPED
when calling it.
So let's punt the introduction of anonymous pipes*, but that doesn't mean we should just ignore that they might be introduced in the future in how we decide to name things - which it's probably time to decide now.
*: building your own CreatePipe implementation is quite straight forward.
This reverts commit ef03f09.
To start out, we could have a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! My only quibble is the peek
function (comment inline). It also could be possible to remove it for the initial PR and add it back later once we pick an API.
Thanks for all of the efforts on this. I was able to replace the native Win32 implementation in our project with this to test it out and it worked like a charm! I look forward to seeing it in an upcoming release. |
Agreed, our project was working on a horrifying half-baked blocking implementation to get a UDS equivalent on windows working so throwing all that code out and getting clean reliable async IO feels really nice. |
Yeah, we'll use this immediately with Prisma to provide named pipe support for our Windows SQL Server users. |
This introduces a couple of new types:
tokio::net::windows::NamedPipe
tokio::net::windows::NamedPipeBuilder
tokio::net::windows::NamedPipeClientBuilder
tokio::net::windows::PipeMode
It is based on a suggestion I did in #3388, which is to try and push the exported types to the minimum low level of abstraction necessary to enable their use. This is also a working proposal for #3511.
It is also based on the API proposed by @Darksonn here. With some removals and changes.
In particular I try to closely as possible:
async
.WaitNamedPipe
unfortunately only provides a blocking API, so it uses theasyncify
approach thattokio::fs
does and blocks in a worker thread (see comment).This provides two builders, because named pipe clients and servers are constructed differently. Clients through
CreateFile
, and servers (the one creating the named pipe) throughCreateNamedPipe
. I decided to call the one creating the named pipeNamedPipeBuilder
, all though naming is preliminary.For how to use, see the included documentation and examples.
Motivation
Providing named pipes has been stuck for a while, because the current proposal implements a high level model. This PR ports the part from it into a lower level
NamedPipe
type which can either be used directly or to provide higher level APIs like the one suggested in #3388. Once this lands, it can be done external to the project where it can be more easily experimented with, liketokio-util
.Solution
This tries to implement the bare minimum
async
wrapping necessary to use named pipes while providing a minimum level of ergonomics and sanity checking.Note that even if we don't end up going with exporting low level APIs like this, hopefully this can be used as a cleaner internal API to build something else.
Builders taking
self
I modified the builders to take
self
at one point instead of&mut self
, because the following pattern seems more common when building named pipes:And if it returned
&mut Self
, it wouldn't be as convenient to use the functional style. But feel free to provide your input.