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

io: bring back split utility #1521

Merged
merged 3 commits into from
Aug 31, 2019
Merged

io: bring back split utility #1521

merged 3 commits into from
Aug 31, 2019

Conversation

carllerche
Copy link
Member

Bring back split utility as a free fn instead of a method on
AsyncRead. This utility wraps the stream in an Arc and uses mutual
exclusion to ensure correct access.

Additionally, the specialized split_mut fn on TcpStream and UdsStream
is promoted to split.

Bring back `split` utility as a free fn instead of a method on
`AsyncRead`. This utility wraps the `stream` in an `Arc` and uses mutual
exclusion to ensure correct access.

Additionally, the specialized `split_mut` fn on TcpStream and UdsStream
is promoted to `split`.
tokio-io/src/split.rs Outdated Show resolved Hide resolved
tokio-io/src/split.rs Outdated Show resolved Hide resolved
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@carllerche carllerche merged commit 2f91c85 into master Aug 31, 2019

struct Inner<T> {
locked: AtomicBool,
stream: UnsafeCell<T>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a quick check, is UnsafeCell necessary here?

@jonhoo
Copy link
Sponsor Contributor

jonhoo commented Sep 13, 2019

I'm generally happy for this change, but the loss of an owned split for TcpStream that doesn't require locking is pretty sad. Any particular reason why that was removed?

@vojtechkral
Copy link
Contributor

vojtechkral commented Sep 25, 2019

@jonhoo +1, also not sure why the owning version was removed/replaced. I can't use the referencing ReadHalf in my code... @carllerche thoughts? cc @sopium

@carllerche
Copy link
Member Author

An owning version exists as a free fn: io::split(stream).

The idea is to place emphasis on the async/await friendly zero-cost options.

@vojtechkral
Copy link
Contributor

vojtechkral commented Sep 25, 2019

Hm, ok, but the read and write halves being references reduces the usefulness of this split quite a lot. The TcpStream is really just a socket file descriptor and tcp is fully duplex, so unless I'm missing something, the halves could be owning and it would be just a matter of when to close the fd, which could be done with and Arc without hurting the zero-costness, no?

@carllerche
Copy link
Member Author

You are correct, except that introducing an Arc means it is not zero-cost.

Could you describe more what you are trying to do?

@vojtechkral
Copy link
Contributor

Our code is here (using tokio 0.1 with async/await preview, beware, there are some ugly hacks, especially my attemp at tcp_split()). Basically we need to have separate structures for the read and write halves which have a bunch of stuff on top of them and are used in separate tokio tasks.

I'm in the process of attempting to port that code to tokio 0.2.

So, would it be considered non-zero-cost even if the Arc were to be used only on split() and drop with the I/O going through the file descriptor without indirection? If so, then I don't see a way to do truly zero cost owning variant. But still, we could have an owning variant with much lower cost compared to locking...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants