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: Replace Into<io::Error> bound on StreamMuxer with std::error::Error #2710

Merged
merged 9 commits into from
Jun 24, 2022

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Jun 16, 2022

Description

This allows us to preserve the type information of a muxer's concrete
error as long as possible. For StreamMuxerBox, we leverage io::Error's
capability of wrapping any error that implements Into<Box<dyn Error>>.

Links to any relevant issues

This is a stacked PR on top of #2707 and thus a draft but is otherwise ready to review.

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/introduce-substream-box June 16, 2022 09:23
@thomaseizinger thomaseizinger marked this pull request as ready for review June 16, 2022 09:29
@thomaseizinger thomaseizinger marked this pull request as draft June 16, 2022 09:29
@@ -38,6 +39,7 @@ impl<T> StreamMuxer for Wrap<T>
where
T: StreamMuxer,
T::Substream: AsyncRead + AsyncWrite + Send + Unpin + 'static,
T::Error: Into<Box<dyn Error + Send + Sync>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
T::Error: Into<Box<dyn Error + Send + Sync>>,
T::Error: Error + Send + Sync>,

Nit: Wouldn't this simpler trait bound also work, given that Into<Box<dyn ..>> is auto-implemented for E: Error + Send + Sync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably yes. I've copied the bounds from std::io::Error::new because that is where I am passing the type to. Omitting the Into here is probably okay :)

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 decided to put the Error bound directly on the associated type so I don't have to repeat it everywhere. The Send + Sync bounds stay on higher layers for consistency with other bounds.

Copy link
Member

@mxinden mxinden Jun 21, 2022

Choose a reason for hiding this comment

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

I decided to put the Error bound directly on the associated type so I don't have to repeat it everywhere.

It is only in 4 locations, right? a479eb8 I would prefer bounds to be as close to where they are used as possible. If I am not mistaken StreamMuxer doesn't need type Error to implement Error, right? I liked how previously this pull request would remove the bound on StreamMuxer.

Copy link
Contributor Author

@thomaseizinger thomaseizinger Jun 21, 2022

Choose a reason for hiding this comment

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

It doesn't. It also doesn't necessarily need Substream to implement AsyncRead + AsyncWrite but it is a bit pointless if it doesn't.

If we remove the Error bounds, we should also remove AsyncRead + AsyncWrite for consistency but that will get a bit redundant I think.

The current style seems to be that functional bounds are put on associated types and auto-traits are left off.

@@ -134,7 +134,7 @@ where
pub fn poll(
&mut self,
cx: &mut Context<'_>,
) -> Poll<Result<SubstreamEvent<TMuxer, TUserData>, IoError>> {
) -> Poll<Result<SubstreamEvent<TMuxer, TUserData>, TMuxer::Error>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not in scope for this PR, but while reviewing I was wondering: Do we even need the TMuxer generic on Muxing? I think the actual type is always StreamMuxerBox. Muxing is private the swarm crate and only used by Connection and there we already use the concrete StreamMuxerBox.

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 am going to inline the entire Muxing type into Connection once poll_outbound is removed from StreamMuxer. This is already done in #2648 if you want to see what it looks like :)

I am also tempted to inline HandlerWrapper and centralize all the connection handling logic directly in Connection. This would remove quite a bit of indirection because substreams would directly be passed from the StreamMuxer to the ConnectionHandler.

@thomaseizinger thomaseizinger changed the title core/muxing: Remove Into<io::Error> bound from StreamMuxer::Error core/muxing: Replace Into<io::Error> bound from StreamMuxer::Error with std::Error Jun 20, 2022
@thomaseizinger thomaseizinger changed the title core/muxing: Replace Into<io::Error> bound from StreamMuxer::Error with std::Error core/muxing: Replace Into<io::Error> bound on StreamMuxer with std::error::Error Jun 20, 2022
@thomaseizinger thomaseizinger force-pushed the refactor/introduce-substream-box branch from cdfa81e to f014c29 Compare June 20, 2022 15:22
@thomaseizinger
Copy link
Contributor Author

Rebase only to stay up to date with #2707.

Base automatically changed from refactor/introduce-substream-box to master June 23, 2022 11:52
@thomaseizinger
Copy link
Contributor Author

@thomaseizinger thomaseizinger merged commit 0f40e51 into master Jun 24, 2022
@thomaseizinger thomaseizinger deleted the refactor/remove-io-error-bound branch June 24, 2022 06:26
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.

3 participants