http-rs / http-rs/async-h1

Suggestion to remove Clone bound from reader

Open
#175 7 comments 0 reactions 1 assignee Claimed by @yoshuawuyts View on GitHub
Dominant language
Rust
Stars
166
Forks
49
PR merge metrics
No merged PRs in 30d

Description

AIUI, the clone bound is required in the case where multiple requests are accepted over the same AsyncRead and to avoid having a lifetime bound on the `Request` type.

I think there a couple of problems with this:
- Many streams are not cloneable, which can make integrations difficult, eg. #109.
- Even for a TcpStream, it doesn't *really* make sense to clone it, as concurrent acess will just corrupt the state.
- There's nothing that inherently prevents accidental concurrent access: the crate just relies on the caller doing the right thing.

I think a good workaround would be to use a `oneshot::channel`. Specifically, create an `AsyncRead` type like:

```rust
struct LeasedReader {
return_to_sender: oneshot::Sender,
inner: Option,
}
```
This would have a `finish()` method on that returns the inner reader to the sender. The advantage of this is that the sender loses access to the stream whilst is it on lease, the stream does not require any internal synchronization, and if there is a problem whilst reading from the stream (such as a panic or error) it will not be returned to the sender. The sender will just see the oneshot channel be canceled, and can handle that case cleanly.

The downside to this is that it would require changing the signature of the `accept` function: I see two possibilities. Either the sender side can get its own wrapper that implements `AsyncRead` and hides the fact that the inner reader may be leased out. Alternatively, the `accept` function can return an additional `impl Future` (the oneshot::Receiver) for the caller to recover the reader.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.