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

[zonyitoo]

Motivation

#3873

Solution

Exposes two APIs from the underlying registration

• poll_read_io, try_read_io
• poll_write_io, try_write_io

[Darksonn]

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]

Done.

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

[zonyitoo]

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

[Darksonn]

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.

[zonyitoo]

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

[Darksonn]

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

[Darksonn]

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 & Paste these methods to UnixStream, UnixDatagram and UdpSocket.

[Darksonn]

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

[Darksonn]

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]

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

[Darksonn]

There is also a NamedPipeServer socket type.

[Darksonn]

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

[carllerche]

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]

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]

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

[zonyitoo]

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]

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

[Darksonn]

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]

Still waiting @carllerche .

[Darksonn]

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]

Done.

[Darksonn]

Thanks for your patience with the many changes.

[zonyitoo]

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

[Darksonn]

All PRs are squashed on merge.

[zonyitoo]

Excellent! When will be the next release? @Darksonn

[Darksonn]

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