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: allow customized I/O operations for TcpStream (#3873) #3888

Merged
merged 11 commits into from
Jul 8, 2021
Merged

net: allow customized I/O operations for TcpStream (#3873) #3888

merged 11 commits into from
Jul 8, 2021

Conversation

zonyitoo
Copy link
Contributor

@zonyitoo zonyitoo commented Jun 25, 2021

Motivation

#3873

Solution

Exposes two APIs from the underlying registration

  • poll_read_io, try_read_io
  • poll_write_io, try_write_io

zonyitoo added a commit to shadowsocks/shadowsocks-rust that referenced this pull request Jun 25, 2021
zonyitoo added a commit to shadowsocks/shadowsocks-rust that referenced this pull request Jun 25, 2021
zonyitoo added a commit to shadowsocks/shadowsocks-rust that referenced this pull request Jun 25, 2021
zonyitoo added a commit to shadowsocks/shadowsocks-rust that referenced this pull request Jun 25, 2021
zonyitoo added a commit to shadowsocks/shadowsocks-rust that referenced this pull request Jun 25, 2021
zonyitoo added a commit to shadowsocks/shadowsocks-rust that referenced this pull request Jun 25, 2021
@Darksonn
Copy link
Contributor

Why poll_write_io over try_io? The latter seems more powerful as you can use it to get an poll_write_io by combining it with poll_write_ready.

zonyitoo added a commit to shadowsocks/shadowsocks-rust that referenced this pull request Jun 25, 2021
@zonyitoo

This comment has been minimized.

@zonyitoo
Copy link
Contributor Author

zonyitoo commented Jun 25, 2021

Done.

I am not exposing try_io directly just because there is no poll_io in Registration. APIs should be consistent, right?

zonyitoo added a commit to shadowsocks/shadowsocks-rust that referenced this pull request Jun 25, 2021
zonyitoo added a commit to shadowsocks/shadowsocks-rust that referenced this pull request Jun 25, 2021
zonyitoo added a commit to shadowsocks/shadowsocks-rust that referenced this pull request Jun 25, 2021
zonyitoo added a commit to shadowsocks/shadowsocks-rust that referenced this pull request Jun 25, 2021
zonyitoo added a commit to shadowsocks/shadowsocks-rust that referenced this pull request Jun 25, 2021
zonyitoo added a commit to shadowsocks/shadowsocks-rust that referenced this pull request Jun 25, 2021
@zonyitoo
Copy link
Contributor Author

zonyitoo commented Jun 26, 2021

@Darksonn Please take a look again at this simple PR.

@Darksonn
Copy link
Contributor

It isn't so simple. We have to think about the shape of the methods to make sure that we don't regret adding these methods. Additionally, if we do want them, we would also want them on UnixStream and the two named pipe types, and probably also something similar for UnixDatagram and UdpSocket.

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-net Module: tokio/net labels Jun 28, 2021
@zonyitoo
Copy link
Contributor Author

zonyitoo commented Jul 1, 2021

@Darksonn It has been a week, what's the status? What can I do for help, for example, adding try_ios for other structs.

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.

I think the suggested API looks ok except that it should use FnOnce, not FnMut.

tokio/src/net/tcp/stream.rs Outdated Show resolved Hide resolved
tokio/src/net/tcp/stream.rs Outdated Show resolved Hide resolved
tokio/src/net/tcp/stream.rs Outdated Show resolved Hide resolved
tokio/src/net/tcp/stream.rs Outdated Show resolved Hide resolved
tokio/src/net/tcp/stream.rs Outdated Show resolved Hide resolved
@Darksonn
Copy link
Contributor

Darksonn commented Jul 1, 2021

What can I do for help, for example, adding try_ios for other structs.

That would be great. We would want it on all of the socket types.

@zonyitoo
Copy link
Contributor Author

zonyitoo commented Jul 1, 2021

Copy & Paste these methods to UnixStream, UnixDatagram and UdpSocket.

@Darksonn
Copy link
Contributor

Darksonn commented Jul 1, 2021

There are also two more socket types in the named pipe module.

@Darksonn
Copy link
Contributor

Darksonn commented Jul 1, 2021

It seems like changes were made to the named pipe file after you first submitted this PR. Can you merge or rebase on master?

@zonyitoo
Copy link
Contributor Author

zonyitoo commented Jul 1, 2021

Of course. Are there any other modifications have to be made?

tokio/src/net/tcp/stream.rs Outdated Show resolved Hide resolved
tokio/src/net/tcp/stream.rs Outdated Show resolved Hide resolved
@Darksonn
Copy link
Contributor

Darksonn commented Jul 1, 2021

There is also a NamedPipeServer socket type.

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.

Besides these (and similar in other files), the documentation looks good 👍

tokio/src/net/tcp/stream.rs Outdated Show resolved Hide resolved
tokio/src/net/tcp/stream.rs Outdated Show resolved Hide resolved
tokio/src/net/tcp/stream.rs Outdated Show resolved Hide resolved
tokio/src/net/tcp/stream.rs Outdated Show resolved Hide resolved
tokio/src/net/tcp/stream.rs Outdated Show resolved Hide resolved
@carllerche
Copy link
Member

I am not exposing try_io directly just because there is no poll_io in Registration. APIs should be consistent, right?

I would not weigh the current Registration too heavily. That is an internal API that has evolved over time to support the public API.

There are a lot of new functions proposed here. Can you tell me why we need to add many new functions instead of a single try_io function? I skimmed the discussion but did not see a technical reason.

@zonyitoo
Copy link
Contributor Author

zonyitoo commented Jul 2, 2021

Just 4 functions for each network I/O structs:

  1. try_read_io, try_write_io
  2. poll_read_io, poll_write_io

All the original network I/O structs, like TcpStream already has

  1. ready which accepts Interest
  2. readable, writable async functions
  3. poll_read_ready, poll_write_ready poll functions
  4. try_read, try_write, try_read_vectored, try_write_vectored

All of these functions except ready have two versions, reading and writing. So for API consistency, I added the 4 functions in read and write pairs.

The try_*_io is just for clearing the readiness state when the user provided function returns WouldBlock. Users will (usually) have to use it with readable, poll_read_ready, ... for checking the internal I/O object's readiness state. It is more flexible but users would have to take care about more details.

The poll_*_io is equivalent to calling poll_*_ready and then try_*_io and then wrapping in a loop. These functions will help users to write their code without taking care about when to return Poll::Pending and Poll::Ready.

@Darksonn
Copy link
Contributor

Darksonn commented Jul 2, 2021

I think I agree that taking out the poll_*_io methods is best. We can document how to properly use the try methods.

@zonyitoo
Copy link
Contributor Author

zonyitoo commented Jul 3, 2021

I think I agree that taking out the poll_*_io methods is best. We can document how to properly use the try methods.

Ok. Let me remove them.

@zonyitoo
Copy link
Contributor Author

zonyitoo commented Jul 5, 2021

@Darksonn @carllerche Any other changes have to be made for this PR?

@Darksonn
Copy link
Contributor

Darksonn commented Jul 5, 2021

The implementation and documentation seems fine. I don't have any other changes at the moment. Thanks for being patient with the large list of changes.

I'd like to discuss with @carllerche, but it's currently a holiday in the US.

@zonyitoo
Copy link
Contributor Author

zonyitoo commented Jul 7, 2021

Still waiting @carllerche .

@Darksonn
Copy link
Contributor

Darksonn commented Jul 7, 2021

I talked with @carllerche. I think we would prefer to merge them into a single try_io, similar to the one on Registration. We have a really large number of methods on the socket types already, and we want to minimize the number of additions to the list. Furthermore, this specific API is quite low-level, so it doesn't matter that it isn't the most ergonomic to use.

@zonyitoo
Copy link
Contributor Author

zonyitoo commented Jul 8, 2021

Done.

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.

Thanks for your patience with the many changes.

@zonyitoo
Copy link
Contributor Author

zonyitoo commented Jul 8, 2021

I would advice to squash all these commits to one when merging this PR.

@Darksonn
Copy link
Contributor

Darksonn commented Jul 8, 2021

All PRs are squashed on merge.

@Darksonn Darksonn merged commit c306bf8 into tokio-rs:master Jul 8, 2021
@zonyitoo
Copy link
Contributor Author

zonyitoo commented Jul 8, 2021

Excellent! When will be the next release? @Darksonn

@Darksonn
Copy link
Contributor

Darksonn commented Jul 8, 2021

Our aim is once every two weeks, so probably next week. Currently we have only your change and #3273.

@Darksonn Darksonn mentioned this pull request Jul 16, 2021
atkdef pushed a commit to atkdef/shadowsocks-rust that referenced this pull request Sep 24, 2021
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.

3 participants