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: remove Async{Read,Write}Ext #3005

Closed
carllerche opened this issue Oct 20, 2020 · 9 comments
Closed

io: remove Async{Read,Write}Ext #3005

carllerche opened this issue Oct 20, 2020 · 9 comments
Assignees
Labels
A-tokio Area: The main tokio crate C-proposal Category: a proposal and request for comments M-io Module: tokio/io
Milestone

Comments

@carllerche
Copy link
Member

carllerche commented Oct 20, 2020

All the util functions should be moved to AsyncRead and AsyncWrite.

The same should be applied to all other traits defined by Tokio:

  • AsyncSeek
  • AsyncBufRead
@carllerche carllerche added C-proposal Category: a proposal and request for comments A-tokio Area: The main tokio crate labels Oct 20, 2020
@carllerche carllerche added this to the v1.0 milestone Oct 20, 2020
@Darksonn Darksonn added the M-io Module: tokio/io label Oct 20, 2020
@LucioFranco LucioFranco self-assigned this Dec 10, 2020
@jonhoo
Copy link
Sponsor Contributor

jonhoo commented Dec 11, 2020

What is the rationale for this change?

@carllerche
Copy link
Member Author

@jonhoo The original intent of splitting AsyncRead and AsyncReadExt was that they could be in different crates w/ different levels of stability.

As we reach 1.0, both the helpers and the core trait have the same level of stability.

@aknuds1
Copy link
Contributor

aknuds1 commented Dec 15, 2020

I'd like to work on this one if I may.

@carllerche
Copy link
Member Author

@aknuds1 that should be fine. I would coordinate w/ the stream PR as this might cause some merge conflicts (see which files it modifies): #3277

The idea is to move all the helper fns from the ext traits onto the core traits.

@carllerche
Copy link
Member Author

It's looking like we are going to be unable to move forward on this and will be required to leave the Ext traits as is.

First, moving the fns from *Ext back onto the core trait will prevent using the util methods with trait objects. Second, naming the trait *Ext is a convention listed here: https://github.com/rust-lang/rfcs/blob/master/text/0445-extension-trait-conventions.md

Context:

@aknuds1
Copy link
Contributor

aknuds1 commented Dec 19, 2020

@carllerche Ow, that's too bad :/ Want me to close the PR?

@Darksonn
Copy link
Contributor

I thought we could be saved by implementing AsyncRead for &mut dyn AsyncRead, but we already do that, and calling the methods through that impl is rather inconvenient as I also mentioned on the PR.

For example, this compiles:

async fn send_recv_all(
    mut read: &mut (dyn AsyncRead + Unpin),
    mut write: &mut (dyn AsyncWrite + Unpin),
    input: &[u8],
) -> std::io::Result<Vec<u8>> {
    (&mut write).write_all(input).await?;
    (&mut write).shutdown().await?;

    let mut output = Vec::new();
    (&mut read).read_to_end(&mut output).await?;
    Ok(output)
}

@aknuds1 I assume the Americans have gone to bed now, but yeah we will probably have to close the PR because of this.

@aknuds1
Copy link
Contributor

aknuds1 commented Dec 19, 2020

@Darksonn Yeah making the same assumption :)

@carllerche
Copy link
Member Author

Ok, given the issues hit and the established convention, we are going to leave this as is.

@aknuds1 thanks for your work! Even if it didn’t land it was really helpful to get us to the conclusion.

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-proposal Category: a proposal and request for comments M-io Module: tokio/io
Projects
None yet
Development

No branches or pull requests

5 participants