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: add a copy_bidirectional utility #3230

Closed
wants to merge 2 commits into from

Conversation

bdonlan
Copy link
Contributor

@bdonlan bdonlan commented Dec 8, 2020

Motivation

While there is a unidirectional copy utility currently, bidirectional copying between two streams (eg, proxying connections) is a bit tricky to do seamlessly and correctly due to the need to propagate EOFs and handle various error conditions.

Solution

This change adds a helper that can be used to, for example, glue together two TcpStreams and forward data across them.

@Darksonn Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-io Module: tokio/io labels Dec 8, 2020
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.

The implementation looks correct. All my comments are about code style.

Comment on lines +28 to +30
) -> Poll<io::Result<u64>>
where A: AsyncRead + AsyncWrite + Unpin + ?Sized, B: AsyncRead + AsyncWrite + Unpin + ?Sized
{
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised rustfmt is accepting this. Maybe the cfg_* macro is stopping it?

Please rustfmt the contents of this file.

loop {
match state {
TransferState::Running(buf) => {
*state = TransferState::ShuttingDown(ready!(dbg!(buf.poll_copy(cx, r.as_mut(), w.as_mut())))?);
Copy link
Contributor

Choose a reason for hiding this comment

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

There seems to be a stray dbg! here. I'd also prefer to have the ready! on a separate line to make it more clear that this doesn't unconditionally update the state.


*state = TransferState::Done(*count);
}
TransferState::Done(count) => break Poll::Ready(Ok(*count)),
Copy link
Contributor

Choose a reason for hiding this comment

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

When using a loop in this way, I think this is clearer.

Suggested change
TransferState::Done(count) => break Poll::Ready(Ok(*count)),
TransferState::Done(count) => return Poll::Ready(Ok(*count)),

}
}

impl<'a, A, B> std::future::Future for CopyBidirectional<'a, A, B>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd prefer to import Future.

Comment on lines +4 to +11
use tokio::{
io::{self},
task::JoinHandle,
};

use std::time::Duration;
use tokio::io::{AsyncReadExt, AsyncWriteExt};
use tokio::net::TcpStream;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
use tokio::{
io::{self},
task::JoinHandle,
};
use std::time::Duration;
use tokio::io::{AsyncReadExt, AsyncWriteExt};
use tokio::net::TcpStream;
use std::time::Duration;
use tokio::io::{self, AsyncReadExt, AsyncWriteExt};
use tokio::net::TcpStream;
use tokio::task::JoinHandle;

Comment on lines +60 to +63
let handle = tokio::spawn(async move {
let (r1, r2) = tokio::io::copy_bidirectional(&mut b1, &mut a1).await?;
Ok((r2, r1))
});
Copy link
Contributor

Choose a reason for hiding this comment

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

In the other you just return directly, here you use the question mark. I don't mind either way, but it's nice to be consistent.


#[tokio::test]
async fn test_basic_transfer() {
symmetric(|_handle, mut a, mut b| async move {
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually prefer to join the handles, as we otherwise don't catch panics in the task.

@Darksonn Darksonn added the S-waiting-on-author Status: awaiting some action (such as code changes) from the PR or issue author. label Jan 8, 2021
@Darksonn
Copy link
Contributor

Darksonn commented May 5, 2021

Closing in favor of #3572.

@Darksonn Darksonn closed this May 5, 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 C-enhancement Category: A PR with an enhancement or bugfix. M-io Module: tokio/io S-waiting-on-author Status: awaiting some action (such as code changes) from the PR or issue author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants