impl AsyncRead/AsyncWrite for Arc<T>

[ghost]

In async-std it is common to share I/O types by storing them inside an Arc. It would be nice then for Arc<T> to also be an I/O type by implementing AsyncRead and AsyncWrite. In fact, this is exactly why the io-arc crate was invented: https://docs.rs/io-arc/1.0.0/io_arc/

I'm now working on a new runtime where this pattern is pervasive and am also using a variant of IoArc. It would be nice for futures to add this impl instead.

[Matthias247]

This seems broken for all AsyncRead/AsyncWrite implementations which are only able to store exactly one Waker - or one per direction - which are likely most of them. If they are not then a second poll_read/write on one of the clones will overwrite the stored Waker, which leads the task awaiting the other clone to get stuck.

Unless you can add a constraint that guarantees that the underlying object is able to handle multiple concurrent IOs safely, I don't think this should be added.

[ghost]

@Matthias247 AsyncRead/AsyncWrite can store as many wakers as are needed, not just two.

Both async-std and smol can safely handle any number of concurrent I/O operations.

[Nemo157]

where for<'a> &'a T: AsyncRead at the very least implies that it should support cross-task shared polling, checking Tokio it doesn't appear to provide shared impls, and checking async-std it does and it does appear to support multiple tasks.

My problem with these impls is just that it makes the chance of wrongly interleaved reads/writes higher, I assume in most cases the intention is to just support splitting into a read/write pair, which IMO would be better supported by inherent split methods.

If these are to be provided, should there be the same for AsyncSeek and Stream as well? Stream in particular seems like it avoids the issue I see with the IO impls since it provides atomic units of data instead of reading/writing some unknown amount at a time.

[Matthias247]

Both async-std and smol can safely handle any number of concurrent I/O operations.

Maybe. But letting users guess what is supported just sounds like a bad idea. If a certain implementation supports splitting/cloning, they can directly add methods that indicate that. I am not sure what the benefit of the generalization here is.

And some more background: I think AsyncRead/Write/Stream/Sink had been modelled after the Future type itself. The all take a Context/Waker as argument, and require a mutable pinned reference. Futures are commonly expected not to store multiple Wakers, and rather to replace the stored Waker on each poll.

[ghost]

My problem with these impls is just that it makes the chance of wrongly interleaved reads/writes higher

Can you elaborate what is the problem here? How could operations be interleaved wrongly and under what circumstances?

To clarify, I think I understand what you're worried about - it's just that my opinion is that issues in question are so contrived and unrealistic they simply aren't a problem. Not once this came up in async-std's history. And the worst issue I can see is a memory leak. But then again, there are so many other non-contrived memory leaks that make this one not worth thinking about, in my opinion.

If these are to be provided, should there be the same for AsyncSeek and Stream as well?

Yes - why not! :) Channels in async-std and piper are MPMC - their Receivers implement Stream, but it would be trivial to also implement the trait for &Receiver.

But letting users guess what is supported just sounds like a bad idea. If a certain implementation supports splitting/cloning, they can directly add methods that indicate that. I am not sure what the benefit of the generalization here is.

The point of the generalization is that we're forced to re-invent a fundamental type in std and use our own Arc because the futures crate doesn't have these impls yet.

The all take a Context/Waker as argument, and require a mutable pinned reference. Futures are commonly expected not to store multiple Wakers, and rather to replace the stored Waker on each poll.

I fully agree! If T: Future, it can have only one Waker. But if &T: Future, you can have an infinite number of &Ts, and therefore an infinite number of Wakers.

[Nemo157]

By wrong interleaving I mean things like splinched reads

let x = spawn({
  let reader = reader.clone();
  async move {
    let mut buf = [0; 2];
    reader.read_exact(&mut buf).await?;
    Ok(buf)
  }
});
let y = spawn(async move {
  let mut buf = [0; 2];
  reader.read_exact(&mut buf).await?;
  Ok(buf)
});
let (x, y) = try_join(x, y).await?;

Depending on exact timings a data stream sent as [0, 1, 2, 3] could arrive as x: [0, 1], y: [2, 3] or x: [0, 2], y: [1, 3] or a handful of other results.

The same can happen with interleaved writes from concurrent tasks.

[ghost]

@Nemo157 Thanks for the example!

So, I see the issue, but this is also possible with I/O in the standard library, so why apply different standards here?

[Nemo157]

I feel like the std shared IO impls were a mistake too, they might more directly match the underlying platform capabilities, but I've not seen the utility of them compared to the potential for issues (though TBH I've not seen any issues actually arise in real code).

I'm not really against adding these impls, if IO runtimes want to support shared access then having these will just make using those slightly easier, it's the IO runtime's choice to support them.

[najamelan]

This seems broken for all AsyncRead/AsyncWrite implementations which are only able to store exactly one Waker

Those types should not implement AsyncRead/AsyncWrite for &T then. They wouldn't be affected by this PR.

I think providing blanket impls where possible is a feature. It removes friction when designing API's and also when using them.

[taiki-e]

fwiw: This is rust-lang/rust#55058, not 69952

[taiki-e]

looks like there is no known AsyncRead/AsyncWrite implementation that actually supports any number of concurrent I/O operations (smol-rs/async-io#38 (comment)), so I'll close this for now.