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

core/muxing: Force StreamMuxer::Substream to implement Async{Read,Write} #2707

Merged
merged 10 commits into from
Jun 23, 2022

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Jun 15, 2022

Description

Instead of delegating reading and writing of the substream to the StreamMuxer,
we introduce a SubstreamBox which implements Async{Read,Write}. This is
used as the Substream associated type in StreamMuxerBox. This allows us to
delete the read_substream etc functions from the StreamMuxer trait.

Links to any relevant issues

Extracted out of #2648.

Open Questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@thomaseizinger thomaseizinger changed the base branch from master to refactor/mplex-async-read-write-stream June 15, 2022 19:11
Comment on lines -450 to -476
loop {
match this.shutdown_state {
ShutdownState::Shutdown => match this.muxer.shutdown_substream(cx, s) {
Poll::Ready(Ok(())) => this.shutdown_state = ShutdownState::Flush,
Poll::Ready(Err(err)) => return Poll::Ready(Err(err.into())),
Poll::Pending => return Poll::Pending,
},
ShutdownState::Flush => match this.muxer.flush_substream(cx, s) {
Poll::Ready(Ok(())) => this.shutdown_state = ShutdownState::Done,
Poll::Ready(Err(err)) => return Poll::Ready(Err(err.into())),
Poll::Pending => return Poll::Pending,
},
ShutdownState::Done => {
return Poll::Ready(Ok(()));
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is important to focus on this bit of deleted code here.

Previously, it would have been very likely that a substream created by any StreamMuxer implementation would have gone through this logic because using the StreamMuxer interface without the help of SubstreamRef would be extremely cumbersome.

With this PR, substreams are forced to implement AsyncRead and AsyncWrite and thus, users can directly use them. Even if we put this piece of logic into SubstreamBox, it may be bypassed. The mplex implementation follows the pattern of "first close, then flush" anyway and is also separately tested.

My take here is: It is fine to delete this code as long as we document that all implementations of StreamMuxer must create substreams that flush after closing. If we ever get to do #508, this could be part of that test suite. Perhaps we can extract such a test suite out of the mplex tests.

Copy link
Member

Choose a reason for hiding this comment

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

"first close, then flush" anyway and is also separately tested.

I am a bit confused here. My intuition would be: A call to poll_close first flushes the substream before closing it. Same as documented e.g. in Tokio's `AsyncWrite:

Invocation of a shutdown implies an invocation of flush. Once this method returns Ready it implies that a flush successfully happened before the shutdown happened. That is, callers don’t need to call flush before calling shutdown. They can rely that by calling shutdown any pending buffered data will be written out.

https://docs.rs/tokio/1.6.0/tokio/io/trait.AsyncWrite.html#tymethod.poll_shutdown

Is this not the convention in libp2p? Based on the above it is not. Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this not the convention in libp2p?

At least for mplex, no:

/// > **Note**: As opposed to `poll_close()`, a flush it not implied.

I am not 100% sure why this code was added on this abstraction level. With your quote from above, I think this PR is actually making things more correct then because the now added AsyncWrite impl on libp2p_mplex::Substream implies flush on close.

Maybe @tomaka can provide some background why this was solved on a generic level previously?

@thomaseizinger
Copy link
Contributor Author

@elenaf9 @mxinden This is stacked on top of #2706 but can be reviewed independently. It is a draft because I still need to update the docs, otherwise it is ready.

@thomaseizinger thomaseizinger changed the title core/muxing: Introduce SubstreamBox which implements Async{Read,Write} core/muxing: Force StreamMuxer::Substream to implement Async{Read,Write} Jun 16, 2022
Base automatically changed from refactor/mplex-async-read-write-stream to master June 20, 2022 04:42
@thomaseizinger
Copy link
Contributor Author

Hmm, I thought I am clever by stacking PRs but with our squash merge strategy, those first commits don't get dropped :/

…ite}`

Instead of delegating reading and writing of the substream to the `StreamMuxer`,
we introduce a `SubstreamBox` which implements `Async{Read,Write}`. This is
used as the `Substream` associated type in `StreamMuxerBox`. This allows us to
delete the `read_substream` etc functions from the `StreamMuxer` trait.
@thomaseizinger thomaseizinger force-pushed the refactor/introduce-substream-box branch from cdfa81e to f014c29 Compare June 20, 2022 15:22
@thomaseizinger thomaseizinger marked this pull request as ready for review June 20, 2022 15:23
@thomaseizinger
Copy link
Contributor Author

These are already implied through the associated type.
@thomaseizinger
Copy link
Contributor Author

Docs are updated, this is ready for a full review @elenaf9 @mxinden ! :)

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Just two comments. Otherwise this looks great to me. Very happy to see this happening.

Might want to give @tomaka a bit more time, in case he has opinions.

@@ -1,8 +1,11 @@
# 0.33.1 - unreleased
# 0.34.0 - unreleased
Copy link
Member

Choose a reason for hiding this comment

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

Missing version bump in core/Cargo.toml. I prepared this in 6d4e09a with my very hacky script. Hope this is helpful @thomaseizinger.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, that seems to be based off a different version of master? I applied the commit but it generates a lot of merge conflicts.

Investingating ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, seems to be due to the recent release off master! I've resolved the conflicts :)

core/src/muxing/boxed.rs Outdated Show resolved Hide resolved
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Feel free to hit the merge button any time.

@thomaseizinger thomaseizinger merged commit eb490c0 into master Jun 23, 2022
@thomaseizinger thomaseizinger deleted the refactor/introduce-substream-box branch June 23, 2022 11:52
umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024
…Write}` (libp2p#2707)

Co-authored-by: Max Inden <mail@max-inden.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove flush_all from docs
2 participants