core/muxing: Fix StreamMuxer::Error to io::Error

[thomaseizinger]

Description

We are already enforcing that the associated type must convert to
io::Error. We might as well just make all functions return an
io::Error directly.

Links to any relevant issues

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

[elenaf9]

Suggested change

	fn to_io_error(e: yamux::ConnectionError) -> io::Error {
	fn into_io_error(e: yamux::ConnectionError) -> io::Error {

https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv

[thomaseizinger]

Good thinking! Will include in libp2p/rust-yamux#135.

[thomaseizinger]

Done: https://github.com/libp2p/rust-yamux/compare/b4ddd2afd6215081b952512ceba27eb45987c1e0..fb6d9ab0ea3406c87dbd076601790e5b4c85e8d3

[thomaseizinger]

Replaced with upstreaming an API change: libp2p/rust-yamux#135

[thomaseizinger]

Re-opening because there was a misunderstanding in what will be upstreamed.

[thomaseizinger]

I'd like to merge #2703 before I start fixing the conflicts here, also because this depends on libp2p/rust-yamux#136.

[thomaseizinger]

I'd like to merge #2703 before I start fixing the conflicts here, also because this depends on libp2p/rust-yamux#136.

Technically it doesn't depend on it actually but it would make sense to only touch this code once and use the upstream From implementation directly.

[mxinden]

We are already enforcing that the associated type must convert to io::Error. We might as well just make all functions return an io::Error directly.

Sorry for only commenting now. I am undecided whether I agree with the reasoning. I don't expect many people to implement StreamMuxer, thus I am not sure the convenience is worth the lost type information.

I am guessing the argument here is that the concrete error would not bubble up all the way to the user anyways?

[thomaseizinger]

I am guessing the argument here is that the concrete error would not bubble up all the way to the user anyways?

The argument is that we are already forcing the associated Error type to convert into io::Error to be able to construct StreamMuxerBox (which needs a concrete error type). We can keep the associated type but then I would suggest to move the bound from the associated type to the constructor of StreamMuxerBox.

In libp2p-swarm, we are only dealing with StreamMuxerBox and thus always have std::io::Error as the error type.

[elenaf9]

I am guessing the argument here is that the concrete error would not bubble up all the way to the user anyways?

The argument is that we are already forcing the associated Error type to convert into io::Error to be able to construct StreamMuxerBox (which needs a concrete error type). We can keep the associated type but then I would suggest to move the bound from the associated type to the constructor of StreamMuxerBox.

In libp2p-swarm, we are only dealing with StreamMuxerBox and thus always have std::io::Error as the error type.

Third option would be to not have a trait bound, but instead in StreamMuxerBox create a io::ErrorKind::Other for the inner error. That's how it is done in transport::Boxed:

rust-libp2p/core/src/transport/boxed.rs

Line 85 in 04f31cd

.map_err(|e| e.map(box_err))?;

rust-libp2p/core/src/transport/boxed.rs

Lines 137 to 139 in 04f31cd

	fn box_err<E: Error + Send + Sync + 'static>(e: E) -> io::Error {
	io::Error::new(io::ErrorKind::Other, e)
	}

Not saying that this is the best solution (if the inner error also has a io::Error variant it would still wap it in an ErrorKind::Other, which is not ideal), just wanted to throw that option in. If you decide to go for the trait bound I'd propose to change it for transport::Boxed as well.

[thomaseizinger]

I am guessing the argument here is that the concrete error would not bubble up all the way to the user anyways?

The argument is that we are already forcing the associated Error type to convert into io::Error to be able to construct StreamMuxerBox (which needs a concrete error type). We can keep the associated type but then I would suggest to move the bound from the associated type to the constructor of StreamMuxerBox.
In libp2p-swarm, we are only dealing with StreamMuxerBox and thus always have std::io::Error as the error type.

Third option would be to not have a trait bound, but instead in StreamMuxerBox create a io::ErrorKind::Other for the inner error. That's how it is done in transport::Boxed:

I think this is actually my favourite out of all the proposed ideas. What do you think @mxinden? This would also improve consistency within the codebase. Thanks for the pointer @elenaf9, I should have really looked at transport::boxed myself :)

[thomaseizinger]

What do you think of #2710 as an alternative? This is using the strategy suggested by @elenaf9 and what is used in transport::boxed.

[mxinden]

What do you think of #2710 as an alternative? This is using the strategy suggested by @elenaf9 and what is used in transport::boxed.

I like #2710 the best.

We can keep the associated type but then I would suggest to move the bound from the associated type to the constructor of StreamMuxerBox.

Would it be worth including this in #2710 as well? In other words, would it be worth it delegating into-io-error-conversion to the error itself by requiring T::Error: Into<io::Error>, on StreamMuxerBox instead of wrapping each T::Error in an io::Error::new(io::ErrorKind::Other like it is done in into_io_error?

[thomaseizinger]

Closing this in favor of #2710.

[thomaseizinger]

We can keep the associated type but then I would suggest to move the bound from the associated type to the constructor of StreamMuxerBox.

Would it be worth including this in #2710 as well? In other words, would it be worth it delegating into-io-error-conversion to the error itself by requiring T::Error: Into<io::Error>, on StreamMuxerBox instead of wrapping each T::Error in an io::Error::new(io::ErrorKind::Other like it is done in into_io_error?

I am not a fan. io::Error delegates to the inner error anyway so there isn't any unnecessary wrapping and this way, implementations of StreamMuxer can stay unaware that they will ever be converted into an io::Error.

[mxinden]

But what if the inner error is already an io::Error @thomaseizinger?

[thomaseizinger]

But what if the inner error is already an io::Error @thomaseizinger?

Then it will effectively behave as that io::Error in regards to Display-impl, source, etc.

[thomaseizinger]

See https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=4a6d9a901bf523ae50f34ecf2227f903.

[mxinden]

👍 Sounds good. Thanks for explaining @thomaseizinger!