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

Add ready/try methods to NamedPipeClient #3866

Merged
merged 8 commits into from Jun 28, 2021

Conversation

Jake-Shadle
Copy link
Contributor

Motivation

As discussed in other issues related to named pipes, a very common use case for named pipes is using them similarly to unix domain sockets, however the initial named pipe support in 1.7 was intentionally bare bones and thus didn't support doing both read and write operations on the same pipe in the same task, but which is supported for UnixStream and other net types via the ready/try_read/try_write etc family of methods.

Solution

This PR just copies the following methods from UnixStream to NamedPipeClient with 0 changes other than in the doc comments, as well as adding an example and a test.

The ready/try_read/write are usually paired with other methods, so added
those as well. Copied docs from UnixStream and fixed them up
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-net Module: tokio/net labels Jun 17, 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.

You have only added the methods for the NamedPipeClient, but there's also a NamedPipeServer.

@Jake-Shadle
Copy link
Contributor Author

This was intentional just to keep it to the smallest cohesive change, I wanted to avoid stalling the PR as much as possible, but I can of course add those as well if you would like.

@davidpdrsn davidpdrsn requested a review from Darksonn June 22, 2021 12:40
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.

The methods and documentation look fine.

tokio/tests/named_pipe.rs Outdated Show resolved Hide resolved
@carllerche
Copy link
Member

LGTM, ideally we could get @udoprog to weigh in.

Copy link
Contributor

@udoprog udoprog left a comment

Choose a reason for hiding this comment

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

Thanks!

Looks good to me as well. There are some minor documentation inconsistencies, but since they're essentially carried over from copying the documentation of TcpStream (or similar) I don't really find that particularly offensive.

One note is that it would be nice to get less functions that are no_run to get improved test coverage, but it would require that tests to spin up a server side as well which might be a bit noisy.

Just nits and no blockers though as far as I'm concerned since they're all documentation. The set of methods added makes perfect sense to me.

examples/named-pipe-ready.rs Outdated Show resolved Hide resolved
tokio/src/net/windows/named_pipe.rs Outdated Show resolved Hide resolved
@Jake-Shadle
Copy link
Contributor Author

I went through and cleaned up all of the docs, they were all copied from UnixStream, which seems to use socket/stream interchangeably in its docs, but now all the docs for the namedpipeclient just use pipe.

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

4 participants