Add initial named pipes support #3388

blackbeam · 2021-01-06T22:03:11Z

This PR adds basic named pipes support.

Motivation

Solution

Implementation directly uses miow::NamedPipeBuilder to avoid making a PR for mio.

It provides two public types:

NamedPipeServer – which is a server implementation with the familiar accept() method.
NamedPipe – that represents client or server connection.

NamedPipeServer will hold at least one free instance of a pipe to maintain its existence. Mutex is used within accept() (only for mem::swap) to keep it shared. NamedPipeServer::new() will create the first instance with FILE_FLAG_FIRST_PIPE_INSTANCE flag to avoid named pipe instance creation race condition.

NamedPipe wraps mio::NamedPipe and provides the connect() method for client-side connections. connect() will wait for a server instance using the approach similar to a one used in .NET, namely it'll call WaitNamedPipe with default timeout using the spawn_blocking (I couldn't find a better solution). Unlike .NET, connect() won't wait if pipe doesn't exist and will error immediately.

@udoprog, regarding disconnect() – NamedPipe doesn't provide it since it won't play well with NamedPipeServer, one should simply drop an instance. Also I don't think that we should call it in poll_shutdown.

@fussybeaver, regarding security_qos_flags - implementation unconditionally adds SECURITY_IDENTIFICATION

Other thoughts

Maybe it's too high level.

Related issues

#3118

Darksonn

I only quickly looked, but I have this comment.

tokio/src/macros/cfg.rs

udoprog · 2021-01-08T09:20:55Z

tokio/src/net/windows/named_pipe.rs

+        Poll::Ready(Ok(()))
+    }
+
+    fn poll_shutdown(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll<Result<()>> {


Should we call self.disconnect() here?

Seems yes but only for the server side. For me pool_shutdown itself is a bit diverges with the fact, that it's possible for NamedPipe to be reconnected after disconnect(), so I'm not sure about this. I'll try to change the API to fix this.

udoprog · 2021-01-08T10:00:27Z

tokio/src/net/windows/named_pipe.rs

+            .read(true)
+            .write(true)
+            .custom_flags(FILE_FLAG_OVERLAPPED)
+            .open(addr.as_ref())?;


If I understand correctly, the goal here is to omit the FILE_FLAG_FIRST_PIPE_INSTANCE to make this open-only and not create-only?

We should probably add this to mio at some point.

We should probably add this to mio at some point.

Yeah, I meant exactly this saying that mio's interface limits the miow named pipes implementation (miow supports this flag). I wonder if it's OK to simply add From<miow::NamedPIpe> for NamedPipe into mio to not to rewrite miow's NamedPipeBuilder 🤔.

Regarding this open call – I believe FILE_FLAG_FIRST_PIPE_INSTANCE is irrelevant here, since this call leads to OpenFile2.

Regarding this open call – I believe FILE_FLAG_FIRST_PIPE_INSTANCE is irrelevant here, since this call leads to OpenFile2.

Sorry for the confusion. I was asking to clarify if I read correctly that the intent with creating the file object ourselves was to avoid this flag. Since miow would otherwise set it (and we don't have a way of controlling that through mio).

fussybeaver · 2021-01-09T09:15:03Z

tokio/src/net/windows/named_pipe.rs

+        let file = OpenOptions::new()
+            .read(true)
+            .write(true)
+            .custom_flags(FILE_FLAG_OVERLAPPED)


Should we be setting security_qos_flags here?

blackbeam · 2021-01-13T06:55:49Z

@udoprog, @Darksonn, @fussybeaver, please see updated PR message.

carllerche · 2021-01-14T18:42:31Z

I need to better understand the behavior of named pipes. Do they follow the client/server model exclusively? It looks like they can be configured to be unidirectional or bidirectional? Is there anything else to know?

blackbeam · 2021-01-15T07:59:51Z

@carllerche,

The actual set of features is wider than exposed by this PR. It includes:

an ability to control pipe access rights and audit rules
an ability to choose the direction (in, out, in-out)
an ability to impersonate a client (quite dangerous if misused)
an ability to set the maximum number of server instances (1 to 254)
anonymous pipes (implemented in miow)
the Message transmission mode (not supported by anonymous pipes)

This list may not be complete since I'm not an expert.

jadamcrain · 2021-01-15T18:10:58Z

Can someone here comment on how/if this PR will allow serial ports to be integrated in Tokio on Windows? I would be happy to do some testing if its believed this PR now makes this possible.

tokio-serial lost the ability to integrate serial ports on Windows in 1.0, and I am keen to restore this functionality:

berkowski/tokio-serial#33

Mythra · 2021-01-28T04:15:25Z

Hey All,

Looking to do a communication with a server over named pipes, and unfortunately as seen above it's broken with tokio 0.3 and above (including 1.0). Curious if there's any help with testing, code, review, etc. I can do to help push this along.

Thanks for all your owrk!

blackbeam · 2021-01-28T06:50:48Z

Hi, @Mythra.

Curious if there's any help with testing, code, review, etc. I can do to help push this along.

I'm interested in both testing and code/design review. Any feedback would be appreciated.
It seems like the main problem here is not the impl but the API choice, especially because tokio is 1.0 now.

Mythra · 2021-01-28T19:57:32Z

Hey, I've been testing this for my needs (on my own branch I just cherrypicked your commit), and I'd like to report it works just like I'd expect it too.

I won't speak too much on the API Front (as i'm only using a client to a pre-existing named pipe, and am also not a maintainer of tokio), but for comparison between named pipes, and UDS support the code looks effectively the same (in fact I could probably combine them down to the same structures, with just #[cfg]): https://gist.github.com/Mythra/71a88e40b5a9bd0a5964a96b59fc19ec . So i'd say at least the client side is doing what ideally we'd hope for.

fussybeaver · 2021-02-02T06:51:04Z

Client-side API works fine from my side as well (all windows integration tests passing)

simonbuchan · 2021-02-04T07:58:13Z

Is there any reasonable alternative to integrating this? I don't see how it would work now that you can't hand mio stuff to tokio. It seems there really isn't a way to do even a client (which is a regular windows file handle) with tokio if you need proper simultaneous duplex messages.
Perhaps you could build another async runtime on mio, and have the runtime multiplex them but that doesn't sound fun or efficient?

Darksonn · 2021-02-04T11:32:23Z

Is there any reasonable alternative to integrating this?

To be clear, we do want to support named pipes. We just haven't really had the time to look into how it would work.

blackbeam · 2021-02-04T11:58:39Z

Is there any reasonable alternative to integrating this?

The alternative I see is to integrate a clone of a feature-complete low-level API (say this one) into the tokio. It'll be the perfect API choice in the sense that, besides it's feature-completeness, it'll most likely be stable for eternity. Higher-level APIs, like the one proposed here, might go to external crates.

bbqsrc · 2021-02-04T22:04:04Z

We use Named Pipes in production in an extremely similar way to Unix sockets, so much so that it's almost a drop-in replacement. We run gRPC over Named Pipes on Windows, and UDS on macOS. Works like a charm.

But we can't update to Tokio 1.0 because of this lack of support. As far as API design is concerned, if it behaves as server-client UDS would, that should be sufficient for the majority of real-world named pipe use cases. For the rest, I imagine it can be handled by the ecosystem at large so long as a raw file descriptor is accessible from the API.

faern · 2021-02-04T22:15:21Z

I agree completely with @bbqsrc. We also use it pretty identical to UDS. With the addition that we must be able to set security attributes on the server end to limit access.

simonbuchan · 2021-02-05T05:15:58Z

Third on using it as an alternative to UDS, but it's definitely worth investigating if the relative weirdness of the API is something that can be handled ideally by tokio or should be exposed for a higher level library.

Some thoughts, in no particular order:

The overlapped IO example creates a set of instances then waits for them to connect in parallel, is this functionally something that a user could care about, or can tokio pick the "right" option here? Is this best exposed as a parameter on on OpenOptions equivalent like .parallel_connects(4), or should users that want this be creating a bunch of NamedPipeServer themselves (including an option for FILE_FLAG_PIPE_FIRST_INSTANCE?) and calling .connect().await on them, if that's equivalent performance and more flexible?

I suspect, however, that a lot of the different ways named pipes can work are for allowing apps to use the right level of simplicity or performance for their app, and that there are pretty easy ways for tokio to simply get this right and tuck away the scary bits.

Functionality differences, like the PIPE_TYPE_MESSAGE and PIPE_REJECT_REMOTE_CLIENTS seem to not have too much API impact: e.g. message pipes seem to essentially just ensure read buffers don't cross written buffer boundaries. Duplex vs read or write only might make sense to later implement an equivalent to net::Owned{Read,Write}Half, (nice to have anyway) and add .open_read(path) -> ReadHalf / .open_write(path) -> WriteHalf.

Buffer sizes, security etc, all seem like they would want an equivalent to fs::OpenOptions,

Interestingly, on the client side, the "only" thing actually needed is to add "real" async support to tokio::fs::File and use them overlapped, which is nice for regular files too. Given that the tokio blog lists support for io_uring support to get real async IO on linux files, it might or might not make sense to simply do that instead of doing something specific for named pipe clients.

Despite being the common use case, I think tokio shouldn't provide anything to hide the differences between UDS and named pipes itself, Dealing with establishing that there's a server on the other end, exclusive serving, security, will all have specific approaches that will be in use already or otherwise better suited.

bbqsrc · 2021-02-05T08:52:39Z

Despite being the common use case, I think tokio shouldn't provide anything to hide the differences between UDS and named pipes itself, Dealing with establishing that there's a server on the other end, exclusive serving, security, will all have specific approaches that will be in use already or otherwise better suited.

Yes. To clarify my position, I did not mean that the interface should look the same as UDS. I meant that the feature set necessary for a Named Pipes "stable release" to my mind would be the equivalent features to UDS, regardless of the API it reveals.

Xanewok · 2021-02-05T11:30:29Z

I would like to echo what others said - simple client/server with security attributes is what would allow https://github.com/nikvolf/parity-tokio-ipc (simple, unified UDS/named pipes interface) to upgrade to Tokio 1.0.

See https://github.com/NikVolf/parity-tokio-ipc/blob/master/src/win.rs for some prior implementation (we basically reimplement stuff on top of PollEvented<mio_named_pipes::NamedPipe>).

bbqsrc · 2021-02-05T13:33:00Z

And another implementation we use: https://github.com/bbqsrc/tokio-named-pipe (not to be confused with tokio-named-pipes above, hehe)

carllerche · 2021-02-06T22:31:49Z

@simonbuchan

Despite being the common use case, I think tokio shouldn't provide anything to hide the differences between UDS and named pipes itself, Dealing with establishing that there's a server on the other end, exclusive serving, security, will all have specific approaches that will be in use already or otherwise better suited.

Can you clarify these use cases? We need a clear understanding of the use cases in order to model an API.

I think the best strategy for moving forward here is to list out the use cases we want the API to support. This probably should be done in a separate issue. Once we are in agreement w/ the cases we want to cover, then we can start focusing on an API proposal.

blackbeam · 2021-02-27T17:43:17Z

Hi.

The new version introduces NamedPipeBuilder. It also exposes security attributes and accept_remote, requested by @faern and @Xanewok.

It won't work without tokio-rs/mio#1461, so mio version bumped to 0.7.9 (see the immediate_disconnect test). Also, note that the accept() future will resolve successfully even if it is known (to miow) that there is no connection (this fact was hidden from us here). The first call to read/write should reveal the issue.

Regarding connection order. It is possible to conclude empirically, that clients will connect in natural order (see the connection_order test). This behavior is not documented, though.

pimeys · 2021-03-01T14:17:49Z

I just tested this in our SQL Server client Tiberius. Connecting to a pipe and passing this NamedPipe instance to our client works like a charm.

abbec · 2021-03-18T15:57:39Z

Would really love to have this support, what is left for it to be merged? :)

abbec · 2021-03-19T14:16:21Z

Hmm, trying this out, it seems that client/server functionality only works when the server and client is spawned inside the same executable. When the server is a separate executable, accept() never returns. It seems that the mio readiness event from connect_complete never reaches the Future impl on ConnectingInstance

It does however when client and server are in the same process.

tokio/src/net/windows/named_pipe.rs

Mythra · 2021-04-09T13:38:42Z

See both this PR, and the design issue are stalled again. What's left to be done, what can be helped with? Maintaining a fork of this is definitely starting to have it's toll.

Darksonn · 2021-04-10T08:55:58Z

I think the main problem is that to review a PR that implements this, I need to have some understanding of what kind of features the windows API has. I don't want to end up with a sub-par API that only supports some of the use-cases, or one that maps poorly to the underlying windows API. Being able to evaluate this will take a time investment from me that I haven't been able to find the time for yet. There's also the question of crate choice of miow, as we try to avoid having dependencies that are not part of the Tokio project.

Mythra · 2021-04-10T16:24:53Z

That makes sense, it's incredibly unfortunate, but I understand there's only so much time in the day. Thanks for all you do!

simonbuchan · 2021-04-21T03:35:17Z

I just found out about the fact that windows has native support for UDS for about 2 years now!

Can't find an actual release notification, but everyone seems to agree that it works from Windows 1803, and sc query afunix reports it running on my windows machine.

Might be worth enabling that first (assuming it doesn't already work!), if adding native named pipes is too much work.

Mythra · 2021-04-21T04:00:31Z

I'd really dislike prioritizing AF_Unix windows support, for named pipe support. When many apps I've checked as well as cases mentioned in this PR can't use AF_Unix. If contribution time on Windows is low (which seems to be the core problem), using that valuable time doing something not well supported seems not worth it. Especially considering this PR blocks many apps from upgrading Tokio as this used to work. Albeit with an interface that was deemed undesirable.

As an example, Docker does not support this even after its release (my use case along with I know two others who fork): moby/moby#36442

MSSQL another large use case (and the opener of the PR) also do not support AF_Unix.

While obviously some apps do support af_unix I think this is overall a relatively minor use case, and while an issue should be opened. I'd plead that this PR be prioritized above sockets even if it's "quick".

simonbuchan · 2021-04-21T05:20:12Z

Sure, I was wondering if it was close to "just remove the #[cfg(unix)]", but checking it a bit further, it's a bit more than that, tokio just wraps mio here (as expected) and mio is doing a lot of per-AF-and-OS code, rather than trying to facade winsock2 and libc's sockets then building TCP, UDP and UDS on top of that. If it were, then unblocking that seems like it would have been easy enough to unblock the users that just wanted internal IPC and can require a recentish windows, which seems like it would be at least a few.

udoprog · 2021-04-22T08:40:53Z

Just a very high level thought on the stuck nature of this pull request and how we might be able to unstuck it.

It might be easier right now to temporarily abandon the client / server stream architecture that's being proposed and focus more on exposing an API which more closely reflects the underlying low-level API - even if it ends up having unsafe edges (like how the constructor that can take SECURITY_ATTRIBUTES must have). Then, on top of this it might be feasible to experiment with more high level approaches. Like the one proposed in this PR.

That raises another issue in that we need access to a named pipe builder to give users of the API somewhat idiomatic access to a full range of capabilities. And due to the stability guarantees we can't re-export types which are provided by mio or miow, so it would have to live in Tokio until both of them are stable. One way to move forward here could be to copy the builder that's in miow so it can be exported and subject to the stability guarantees of Tokio.

I would at least be able to review such a PR, since I'm more familiar with how the raw API works, and less so with the proposed high-level abstractions.

What do you think @blackbeam @Darksonn?

udoprog · 2021-04-22T08:49:21Z

It's also worth noting that SECURITY_ATTRIBUTES would be a type provided by the unstable winapi crate. And if we provide setting it through a builder like miow that would be part of Tokio's public API. So I'm not entirely sure how to best cope with that other than unsafe raw constructors.

carllerche · 2021-04-26T17:26:01Z

We can always define our own version of SECURITY_ATTRIBUTES.

Maybe the first step is to list out all syscalls one can make w/ a named pipe.

simonbuchan · 2021-04-26T22:15:59Z

As someone that has spent too long staring at the named pipe docs, the miow API is much clearer and is well designed as a "native" API wrapper, at least as an introduction. The main thing that looks like its missing us create flags, mainly message oriented pipes (vs the "normal" byte stream), which have identical usage other than a guarantee on the packets not getting merged, AFAIK.

In particular it preserves the odd design of creating multiple servers, one for each connection, optionally with the first being created exclusively.

simonbuchan · 2021-04-26T22:18:00Z

Note also that SECURITY_ATTRIBUTES is a general Windows security structure that also applies to nearly everything else, and would be a major undertaking to wrap safely.

Darksonn · 2021-06-29T08:12:21Z

Closing as #3760 was merged.

Darksonn · 2021-06-29T08:13:03Z

I would like to thank everyone for helping us understand the right design for named pipes!

Bump to Tokio 1.0 Closes #1695 Closes #1693 Just enabled by tokio-rs/tokio#3388. Work in progress to prune winapi 0.2, still needs patches upstreamed that migrate `parity-tokio-ipc` and `jsonrpc-*` crates to Tokio 1.0. Let's see what CI has to say, first. Blocked on: - [x] paritytech/parity-tokio-ipc#32 - [x] paritytech/jsonrpc#628

blackbeam mentioned this pull request Jan 6, 2021

Update tokio to 1.0 (fix #132) blackbeam/mysql_async#138

Merged

Darksonn reviewed Jan 6, 2021

View reviewed changes

tokio/src/macros/cfg.rs Outdated Show resolved Hide resolved

Darksonn added A-tokio Area: The main tokio crate M-net Module: tokio/net labels Jan 7, 2021

udoprog reviewed Jan 8, 2021

View reviewed changes

fussybeaver reviewed Jan 9, 2021

View reviewed changes

fussybeaver mentioned this pull request Jan 9, 2021

Release plan for support on tokio v0.3? fussybeaver/bollard#130

Closed

Darksonn mentioned this pull request Jan 9, 2021

Need an interface to plug asynchronous Windows Handle types for serial ports #3396

Open

BusyJay mentioned this pull request Jan 10, 2021

Use the gpg-agent for initial ssh socket support BusyJay/gpg-bridge#2

Closed

blackbeam force-pushed the named-pipes branch from cdc1137 to 0793217 Compare January 13, 2021 05:52

blackbeam force-pushed the named-pipes branch from 0793217 to c9f5fe4 Compare January 13, 2021 06:57

fussybeaver mentioned this pull request Jan 30, 2021

Windows named pipes fussybeaver/bollard#138

Merged

pimeys mentioned this pull request Mar 1, 2021

Using Tiberius with named pipes prisma/tiberius#131

Open

berkowski mentioned this pull request Mar 9, 2021

Windows not freeing ports berkowski/tokio-serial#37

Open

pablosichert mentioned this pull request Mar 19, 2021

chore: Fully upgrade to tokio 1.x vectordotdev/vector#5872

Merged

10 tasks

abbec suggested changes Mar 20, 2021

View reviewed changes

tokio/src/net/windows/named_pipe.rs Outdated Show resolved Hide resolved

Always register for readiness events in ConnectingInstance::poll

c475e71

blackbeam force-pushed the named-pipes branch from 5103eb3 to c475e71 Compare March 25, 2021 13:09

Xanewok mentioned this pull request Apr 18, 2021

Migrate to stable Future trait, async/await and Tokio 0.2 runtime mozilla/sccache#985

Merged

udoprog mentioned this pull request May 7, 2021

net: Provide a raw NamedPipe implementation and builders #3760

Merged

Jake-Shadle mentioned this pull request Jun 17, 2021

Add ready/try methods to NamedPipeClient #3866

Merged

Darksonn closed this Jun 29, 2021

This was referenced Jun 29, 2021

Bump to Tokio 1.0 rust-lang/rls#1741

Merged

Upgrade to Tokio 1.0 (and mio 0.7) paritytech/parity-tokio-ipc#32

Merged

Add initial named pipes support #3388

Add initial named pipes support #3388

Conversation

blackbeam commented Jan 6, 2021 • edited

Motivation

Solution

Other thoughts

Related issues

Darksonn left a comment

Choose a reason for hiding this comment

udoprog Jan 8, 2021

Choose a reason for hiding this comment

blackbeam Jan 9, 2021

Choose a reason for hiding this comment

udoprog Jan 8, 2021

Choose a reason for hiding this comment

blackbeam Jan 9, 2021

Choose a reason for hiding this comment

udoprog Jan 9, 2021 • edited

Choose a reason for hiding this comment

fussybeaver Jan 9, 2021

Choose a reason for hiding this comment

blackbeam commented Jan 13, 2021

carllerche commented Jan 14, 2021

blackbeam commented Jan 15, 2021

jadamcrain commented Jan 15, 2021

Mythra commented Jan 28, 2021

blackbeam commented Jan 28, 2021

Mythra commented Jan 28, 2021

fussybeaver commented Feb 2, 2021

simonbuchan commented Feb 4, 2021

Darksonn commented Feb 4, 2021

blackbeam commented Feb 4, 2021

bbqsrc commented Feb 4, 2021

faern commented Feb 4, 2021

simonbuchan commented Feb 5, 2021

bbqsrc commented Feb 5, 2021

Xanewok commented Feb 5, 2021 • edited

bbqsrc commented Feb 5, 2021

carllerche commented Feb 6, 2021

blackbeam commented Feb 27, 2021

pimeys commented Mar 1, 2021

abbec commented Mar 18, 2021

abbec commented Mar 19, 2021 • edited

Mythra commented Apr 9, 2021

Darksonn commented Apr 10, 2021

Mythra commented Apr 10, 2021

simonbuchan commented Apr 21, 2021

Mythra commented Apr 21, 2021 • edited

simonbuchan commented Apr 21, 2021 • edited

udoprog commented Apr 22, 2021 • edited

udoprog commented Apr 22, 2021 • edited

carllerche commented Apr 26, 2021

simonbuchan commented Apr 26, 2021

simonbuchan commented Apr 26, 2021

Darksonn commented Jun 29, 2021

Darksonn commented Jun 29, 2021

blackbeam commented Jan 6, 2021 •

edited

udoprog Jan 9, 2021 •

edited

Xanewok commented Feb 5, 2021 •

edited

abbec commented Mar 19, 2021 •

edited

Mythra commented Apr 21, 2021 •

edited

simonbuchan commented Apr 21, 2021 •

edited

udoprog commented Apr 22, 2021 •

edited

udoprog commented Apr 22, 2021 •

edited