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

util: add writev-aware poll_write_buf #3156

Merged
merged 4 commits into from Dec 3, 2020
Merged

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Nov 19, 2020

Motivation

In Tokio 0.2, AsyncRead and AsyncWrite had poll_write_buf and
poll_read_buf methods for reading and writing to implementers of
bytes Buf and BufMut traits. In 0.3, these were removed, but
poll_read_buf was added as a free function in tokio-util. However,
there is currently no poll_write_buf.

Now that AsyncWrite has regained support for vectored writes in #3149,
there's a lot of potential benefit in having a poll_write_buf that
uses vectored writes when supported and non-vectored writes when not
supported, so that users don't have to reimplement this.

Solution

This PR adds a poll_write_buf function to tokio_util::io, analogous
to the existing poll_read_buf function.

This function writes from a Buf to an AsyncWrite, advancing the
Buf's internal cursor. In addition, when the AsyncWrite supports
vectored writes (i.e. its is_write_vectored method returns true),
it will use vectored IO.

I copied the documentation for this functions from the docs from Tokio
0.2's AsyncWrite::poll_write_buf , with some minor modifications as
appropriate.

Finally, I fixed a minor issue in the existing docs for poll_read_buf
and read_buf, and updated tokio_util::codec to use poll_write_buf.

Signed-off-by: Eliza Weisman eliza@buoyant.io

@hawkw
Copy link
Member Author

hawkw commented Nov 19, 2020

Looks like rustfmt is failing for code in tokio-macros...which this PR doesn't touch at all? Could it be an upstream formatting change?

@taiki-e
Copy link
Member

taiki-e commented Nov 20, 2020

Could it be an upstream formatting change?

Probably something was fixed in Rust 1.48 stable. (It looks like modules that were not previously formatted correctly are now formatted.)

EDIT: filed #3158

@taiki-e taiki-e added the A-tokio-util Area: The tokio-util crate label Nov 20, 2020
This commit adds `poll_write_buf` and `write_buf` functions to
`tokio_util::io`, analogous to the existing `poll_read_buf` and
`read_buf` functions.

These functions write from a `Buf` to an `AsyncWrite`, advancing the
`Buf`'s internal cursor. In addition, when the `AsyncWrite` supports
vectored writes (i.e. its `is_write_vectored` method returns `true`),
they will use vectored IO.

I copied the documentation for these functions from the docs from Tokio
0.2's `AsyncWrite::poll_write_buf` and `AsyncWriteExt::write_buf`
functions, with some minor modifications as appropriate.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw force-pushed the eliza/poll-write-buf-vectored branch from 4804fba to a124b06 Compare November 20, 2020 20:03
@mzabaluev
Copy link
Contributor

mzabaluev commented Nov 21, 2020

Note that there still is a provided implementation of AsyncWriteExt::write_buf in the tokio crate. Should it also make use of these utilities, or be documented as specifically not making use of vectored output, or be slated for removal in a later API revision?

I think an extension method is more convenient than the free-standing function, while the object safety concerns that led to the removal of AsyncWrite::poll_write_buf do not apply to AsyncWriteExt.

@Darksonn Darksonn added the M-io Module: tokio/io label Nov 22, 2020
@Darksonn
Copy link
Contributor

The write_buf in Tokio should be updated to use vectored IO. This PR is more about the poll_* variant of them not being in Tokio afaik.

@hawkw
Copy link
Member Author

hawkw commented Dec 3, 2020

Per a Discord conversation, I'm going to downscope this PR to just add poll_write_buf in tokio-util. I'll make tokio::io::AsyncWriteExt's write_buf method aware of vectored IO in a separate PR.

Instead, we'll add vectored write support to `tokio`'s
`AsyncWriteExt::write_buf` method. This PR only adds `poll_write_buf`
in `tokio-util`.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw changed the title util: add poll_write_buf and write_buf util: add writev-aware poll_write_buf Dec 3, 2020
@hawkw
Copy link
Member Author

hawkw commented Dec 3, 2020

@carllerche and @Darksonn, I've updated this PR to just add poll_write_buf in tokio-util --- we can add a writev-aware write_buf to AsyncWriteExt in the tokio crate, instead.

This should be ready for a review. It would be nice to publish tokio-util 0.5.1 when this lands; I can prepare the release PR.

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.

Seems ok to me.

@hawkw hawkw merged commit 6472998 into master Dec 3, 2020
@hawkw hawkw deleted the eliza/poll-write-buf-vectored branch December 3, 2020 19:19
hawkw added a commit that referenced this pull request Dec 3, 2020
Added

- io: `poll_read_buf` util fn (#2972).
- io: `poll_write_buf` util fn with vectored write support (#3156).
hawkw added a commit that referenced this pull request Dec 3, 2020
Added

- io: `poll_read_buf` util fn (#2972).
- io: `poll_write_buf` util fn with vectored write support (#3156).
hawkw added a commit that referenced this pull request Dec 3, 2020
### Added

- io: `poll_read_buf` util fn (#2972).
- io: `poll_write_buf` util fn with vectored write support (#3156).

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-util Area: The tokio-util crate M-io Module: tokio/io
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants