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

net: Provide a raw NamedPipe implementation and builders #3760

Merged
merged 59 commits into from Jun 15, 2021

Conversation

udoprog
Copy link
Contributor

@udoprog udoprog commented May 7, 2021

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:

  • Provide as close to 1:1 mapping between the exported functions and their corresponding system calls, each documented with a link to msdn.
  • Where it's supported and makes sense, make those functions async. WaitNamedPipe unfortunately only provides a blocking API, so it uses the asyncify approach that tokio::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) through CreateNamedPipe. I decided to call the one creating the named pipe NamedPipeBuilder, 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, like tokio-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:

let server_builder = NamedPipeBuilder::new("\\\\.pipe\\mypipe")
    .first_pipe_instance(true);

// somewhere else

loop {
    let pipe = server_builder.create()?;
    pipe.connect().await?;
}

And if it returned &mut Self, it wouldn't be as convenient to use the functional style. But feel free to provide your input.

@udoprog udoprog added the M-net Module: tokio/net label May 7, 2021
@udoprog udoprog added the A-tokio Area: The main tokio crate label May 7, 2021
Copy link
Contributor

@Darksonn Darksonn left a 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.

tokio/src/net/windows/named_pipe.rs Outdated Show resolved Hide resolved
tokio/src/net/windows/named_pipe.rs Outdated Show resolved Hide resolved
tokio/src/net/windows/named_pipe.rs Outdated Show resolved Hide resolved
Comment on lines +86 to +90
pub async fn connect(&self) -> io::Result<()> {
loop {
match self.io.connect() {
Ok(()) => break,
Err(e) if e.kind() == io::ErrorKind::WouldBlock => {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

tokio/src/net/windows/named_pipe.rs Outdated Show resolved Hide resolved
@udoprog
Copy link
Contributor Author

udoprog commented May 9, 2021

Disabled cross-platform documentation until something like #3768 has properly landed (see #3275).

@Darksonn
Copy link
Contributor

Darksonn commented May 9, 2021

To start out, we could have a #[cfg(all(docsrs, not(windows)))] struct for each of them that only defines the struct name but no methods or anything. Then we could have its documentation simply link to the windows docs. I think that's an ok compromise.

@udoprog
Copy link
Contributor Author

udoprog commented May 10, 2021

To start out, we could have a #[cfg(all(docsrs, not(windows)))] struct for each of them that only defines the struct name but no methods or anything. Then we could have its documentation simply link to the windows docs. I think that's an ok compromise.

Implemented documentation like this and added stubs for any windows-specific things in their own module doc which should only ever be visible w/ docsrs enabled.

Anything linked in NamedPipe documentation therefore points to things like this (if built on unix):
image

ERROR_ACCESS_DENIED:

image

NotDefinedHere is a newly introduced type which is also only visible with docsrs enabled. It looks like this:

image

Finally, because it relies on using re-exports which are redirected when docsrs is enabled, if documentation is generated on windows, all the documentation links points to the appropriate crates instead of using the documentation stubs.

Copy link
Member

@carllerche carllerche left a 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.

@agneil
Copy link

agneil commented Jun 7, 2021

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.

@simonbuchan
Copy link

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.

@pimeys
Copy link

pimeys commented Jun 14, 2021

Yeah, we'll use this immediately with Prisma to provide named pipe support for our Windows SQL Server users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-net Module: tokio/net
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants