misc/prost-codec: Introduce codec for varint prefixed Protobuf messages #2630
Conversation
| @@ -0,0 +1,21 @@ | |||
| [package] | |||
| name = "prost-codec" | |||
There was a problem hiding this comment.
Something to include in asynchronous-codec perhaps? Feature-flagged of course.
There was a problem hiding this comment.
I guess that would only work if we move the UviBytes into asynchronous-codec as well.
There was a problem hiding this comment.
I guess that would only work if we move the
UviBytesintoasynchronous-codecas well.
Yes. That is the reason why I didn't move it there. Not sure whats best. I feel like using Uvi is a libp2p thing and thus should be inside the rust-libp2p repo. Given that it is a separate crate folks outside of libp2p can still use it though.
There was a problem hiding this comment.
It would probably be cleaner to not have any specific codecs in asynchronous-codec and make it an interface-only crate. In that case it would be clear that all codecs live outside the crate :)
But, it is probably not worthwhile going through the effort of refactoring that now ...
| #[error("Failed to decode response: {0}.")] | ||
| Decode( | ||
| #[from] | ||
| #[source] | ||
| prost::DecodeError, | ||
| ), |
There was a problem hiding this comment.
If you are using #[source], you shouldn't include inner error message yourself, otherwise anyhow and friends will duplicate the message!
There was a problem hiding this comment.
Good point. You mentioned this before, though I seem to have forgotten it :( . Thanks for the pointer!
misc/prost-codec/Cargo.toml
Outdated
| unsigned-varint = { version = "0.7", features = ["asynchronous_codec"] } | ||
|
|
||
| [dev-dependencies] | ||
| prost-build = "0.10" |
There was a problem hiding this comment.
| prost-build = "0.10" | |
| prost-build = "0.10" | |
Nit: newline at end of file.
| .ok_or(prost_codec::Error::Io(std::io::Error::new( | ||
| std::io::ErrorKind::UnexpectedEof, | ||
| "", | ||
| )))??; |
There was a problem hiding this comment.
Maybe also add for dcutr a {inbound, outbound}::UpgradeError::StreamClosed like it was done for identify?
There was a problem hiding this comment.
Good idea. Done with the recent commit.
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| async fn recv<T>(mut socket: T) -> io::Result<IdentifyInfo> | ||
| async fn recv<T>(mut socket: T) -> Result<IdentifyInfo, UpgradeError> | ||
| where | ||
| T: AsyncRead + AsyncWrite + Unpin, | ||
| { | ||
| socket.close().await?; |
There was a problem hiding this comment.
Just to help my understanding: Why do we close the socket here before creating the FramedRead, but don't do this in dcutr on upgrade_inbound?
There was a problem hiding this comment.
The standard identify exchange (not identify push) opens a stream to a remote, never writes on it, and expects the remote to send the response on said stream. Given that the local node never writes on the stream it opened it is safe to close it early. Not sure it is strictly necessary though.
(Also given that the local node never writes on the channel, it can use a FramedRead instead of a Framed.)
In DCUtR upgrade_inbound the remote node sends a request and the local node replies with a response. Closing the stream before creating the Framed would prevent the local node from being able to respond.
Does the above explanation make sense @elenaf9?
There was a problem hiding this comment.
Thanks for the reviews @thomaseizinger and @elenaf9. All comments should be addressed.
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| async fn recv<T>(mut socket: T) -> io::Result<IdentifyInfo> | ||
| async fn recv<T>(mut socket: T) -> Result<IdentifyInfo, UpgradeError> | ||
| where | ||
| T: AsyncRead + AsyncWrite + Unpin, | ||
| { | ||
| socket.close().await?; |
There was a problem hiding this comment.
The standard identify exchange (not identify push) opens a stream to a remote, never writes on it, and expects the remote to send the response on said stream. Given that the local node never writes on the stream it opened it is safe to close it early. Not sure it is strictly necessary though.
(Also given that the local node never writes on the channel, it can use a FramedRead instead of a Framed.)
In DCUtR upgrade_inbound the remote node sends a request and the local node replies with a response. Closing the stream before creating the Framed would prevent the local node from being able to respond.
Does the above explanation make sense @elenaf9?
Description
Extracts the Protobuf en-/decoding pattern into its separate crate and applies it to
libp2p-identify.In case this is accepted, we can update the other crates.
1a8b247 also makes
libp2p-identifyexpose an explicitUpgradeErrorinstead of anio::Error.Links to any relevant issues
#2500
Open Questions
Change checklist