-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Conversation
The ready/try_read/write are usually paired with other methods, so added those as well. Copied docs from UnixStream and fixed them up
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.
You have only added the methods for the NamedPipeClient
, but there's also a NamedPipeServer
.
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. |
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 methods and documentation look fine.
LGTM, ideally we could get @udoprog to weigh in. |
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.
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.
I went through and cleaned up all of the docs, they were all copied from |
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 othernet
types via the ready/try_read/try_write etc family of methods.Solution
This PR just copies the following methods from
UnixStream
toNamedPipeClient
with 0 changes other than in the doc comments, as well as adding an example and a test.ready
readable
poll_read_ready
try_read
try_read_vectored
writable
poll_write_ready
try_write
try_write_vectored