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

Implement Read and Write for Arc<T> for types where &T implements Read and Write, *and* it is safe to do so. #504

Open
nmathewson opened this issue Dec 12, 2024 · 2 comments
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@nmathewson
Copy link

nmathewson commented Dec 12, 2024

Proposal

Problem statement

There's an inconsistency in the standard library at present: Arc<File> implements Read and Write, whereas Arc<T> for several other Read+Write types do not. In some cases (such as for [u8]), it isn't correct to do so (see links below). But for several others, such as TcpStream and UnixStream, it would be correct.

This proposal stands for the general proposition that, when &T implements Read and Write, and when it would be correct for Arc<T> to implement Read and Write, we should implement Read and Write on Arc<T>. It also stands for the specific proposal to implement Read and Write on Arc<TcpStream> and Arc<UnixStream>.

(See also prior discussions at rust-lang/rust#53835 and rust-lang/rust#94744. I'm opening this ACP because I was asked to at #134190.)

(This is my first ACP; please let me know if I've done it wrong.)

Motivating examples or use cases

This is roughly the use case I have, though others exist:

fn split_stream<T>(stream: T) -> (Box<dyn Read>, Box<dyn Write>)
where T: 'static,
     Arc<T>: Read + Write
{
    let arc = Arc::new(stream);
    (Box::new(arc.clone()),  Box::new(arc))
}

// This will work fine:
fn split_file(f:File) -> (Box<dyn Read>, Box<dyn Write>) {
    split_stream(f)

// This will fail:
fn split_tcp(f:TcpStream) -> (Box<dyn Read>, Box<dyn Write>) {
    split_stream(f)
}

(This is only one possible motivation. Prior discussion suggests that implementing Read and Write on Arc<T>, where it is correct to do so, would be useful in general for reasons of consistency.)

Solution sketch

See PR at rust-lang/rust#134190.

At first glance, this could possibly be extended to these types, though I don't know if there is a reason to do so:

  • Arc<Stdout>
  • Arc<Stderr>
  • Arc<PipeWriter>
  • Arc<PipeReader>
  • Arc<ChildStdin>
  • Arc<ChildStdout>
  • Arc<Empty>
  • Arc<Sink>

Alternatives

There are numerous other workarounds that work for the motivating example, without changes to the library.

  1. We can tell the user to use try_clone(), at the cost of using an extra fd, and creating a fallible API.
  2. We can tell the user to define a wrapper type that implements Read + Write on NewType(Arc<T>), specifically for the cases where it is safe to do so. This requires a certain amount of boilerplate.
  3. We can tell the user to define a wrapper type that implements Read + Write on Newtype(Arc<Mutex<T>>) for arbitrary T implementing Read + Write. This requires a certain amount of boilerplate, and prevents simultaneous reads and writes.

Within the standard library, there are other approaches for solving the motivating problem.

  1. The standard library could implement its own version of Tokio's split_owned, returning a separate ReadHalf and WriteHalf for each current duplex Read+Write type it provides. This might be desirable for other reasons (such as performance), but is orthogonal to this proposal.
  2. The standard library could deprecate and eventually remove the Read and Write implementations for &T whenever it is not correct for them to apply to Arc<T>. If it did so, it would then be safe to provide a blanket impl<T> Read for Arc<T> where &T:Read .
  3. The standard library could add a ReadByRef trait (and by analogy a WriteByRef trait) for those types where &T implements Read and it is correct to implement Read for Arc<T>. It could then provide a blanket impl<T:ReadByRef> Read for Arc<T>.

Nonetheless, by analogy to the fact that Arc<File> currently implements Read and Write, I think that this approach would probably be the simplest and least controversial approach.

Links and related work

Prior discussions of impl {Read,Write} for Arc<T> where &T: {Read,Write}:

Existing implementation of impl Read,Write for Arc<File>:

Proposed implementation for Arc<TcpStream>, Arc<UnixStream>

@the8472
Copy link
Member

the8472 commented Dec 12, 2024

Note that if you're leading with "its inconsistent" and "it would be correct" that's generally not a problem statement. There are more things that could be done (valid programs/valid types) than can be stored on the atoms of the universe, most of them just aren't all that useful.

ACPs that were exercises in feature-matrix-filling and motivated by consistency have been rejected before.

So a stronger problem statement and motivation would be good.

@kennytm
Copy link
Member

kennytm commented Dec 12, 2024

Better also point to rust-lang/rust#93044 (comment) showing why just avoiding Arc<[u8]> is not enough.

I think if we further restrict the condition to where T: Read, &T: Read it would be truly valid, assuming that the impl Read for T and &T are equivalent.

T Read for T Read for &T Write for T Write for &T
File
TcpStream
PipeReader - -
PipeWriter - -
Stdin - -
Stdout - -
Stderr - -
UnixStream
[u8]
ChildStderr missing? - -
ChildStdout missing? - -
ChildStdin - -
Empty missing?
Repeat missing? - -
Sink - -
VecDeque<u8>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

3 participants