transports/webrtc: Implement stream message framing by mxinden · Pull Request #8 · melekes/rust-libp2p

mxinden · 2022-09-05T03:30:15Z

Description

Implements mxinden/specs#1 for libp2p#2622.

TODOs:

~~Don't use message framing during noise handshake~~ Use message framing during noise handshake (see webrtc/: Add message framing to support half-close and reset of stream mxinden/specs#1)
Update to latest Protobuf
~~Send RESET_STREAM when receiving STOP_SENDING~~ No longer required. See webrtc/: Add message framing to support half-close and reset of stream mxinden/specs#1 (comment).
Enforce maximum message size
Address feedback on test in transports/webrtc: Implement stream message framing #8 (comment)

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

…brtc-message-framing

With mxinden/specs@865f4f2 the RESET no longer resets both write and read part of a stream, but only the former.

thomaseizinger

Thanks for implementing this.

I've left some comments!

thomaseizinger · 2022-09-08T09:09:09Z

+}
+
+impl State {
+    fn handle_flag(&mut self, flag: crate::message_proto::message::Flag) {


Could we import Flag directly? The full-qualified path adds a lot of noise IMO.

👍 done in 11c016f.

thomaseizinger · 2022-09-08T09:13:56Z

+                PollDataChannel {
+                    state: State::ReadClosed { .. },
+                    ..
+                }


Is this consistent with the spec? I think this will just fill-up some internal buffer somewhere. Shouldn't we read into a local variable instead and simply drop that immediately?

Good catch. Though I think we need to solve this differently.

Shouldn't we read into a local variable instead and simply drop that immediately?

For how long would one do that? We have to return Poll::Ready(Ok(0)) at some point to inform the upper layer that the read side closed. After that the upper layer will no longer call AsyncRead::poll_read, thus no longer enabling this implementation to discard incoming data.

I think this will just fill-up some internal buffer somewhere.

I don't think that in itself is a bad thing. We should have back-pressure throughout the entire system and buffer sizes should be reasonable. Thus a full buffer in the levels below should not cause any harm.

That said, say that the read side closed. The remote may still send a STOP_SENDING. We would never receive that message. I think what we should do, iff the read side closed, read on the underlying I/O in the AsyncWrite::poll_write implementation, discarding any messages, though still reacting to any flags from the remote.

Does that reasoning make sense @thomaseizinger? Do you agree? I will prepare a patch, which might make the above reasoning easier to understand.

Promised patch: d46a171

I think this also highlights that this PR needs more tests :)

thomaseizinger · 2022-09-08T09:15:21Z

+                        },
+                    io,
+                } => {
+                    match ready!(io.poll_next_unpin(cx))


Instead of nesting this functionality into here, we could have the match block evaluate to read_buffer and move all this logic below the match. All other match arms have an early return already so this would work.

Thank you. Good idea. Done in 1a6e4bd.

thomaseizinger · 2022-09-08T09:18:13Z

+
+        Pin::new(&mut self.io).start_send(crate::message_proto::Message {
+            flag: None,
+            message: Some(buf.into()),


Shouldn't we enforce the maximum message size here?

👍 Thanks. Done via 9cd4ef7.

thomaseizinger · 2022-09-08T09:22:09Z

+    const PROTO_OVERHEAD: usize = 5;
+
+    #[test]
+    fn proto_size() {


I am not sure I understand the value of this test?

Do we expect protobuf to change how much overhead it is producing?
Additionally, this test diverges from what we do in PollDataChannel. There we use prost_codec::Codec.

Would it make sense to:

bin this test

Enforce the max message size limit in PollDataChannel

Write some tests against PollDataChannel that ensure we actually check the limit

To decouple ourselves from RTCPollDataChannel, we can introduce a generic parameter on our PollDataChannel that defaults to RTCPollDataChannel but is replaced with some dummy buffer in the tests.

Still relevant even though now "Outdated". Tracked in top level pull request description.

thomaseizinger

Thanks! I've left a few more - mostly nitpicking - comments! :)

thomaseizinger · 2022-09-11T10:52:04Z

+        self.io.get_mut().set_read_buf_capacity(capacity)
+    }
+
+    fn io_poll_next(


I'd suggest we move this to a free-function. Associated functions are typically only used for constructors which this isn't which is confusing IMO.

thomaseizinger · 2022-09-11T10:52:44Z

+            Some(Message { flag, message }) => {
+                let flag = flag
+                    .map(|f| {
+                        Flag::from_i32(f).ok_or(io::Error::new(io::ErrorKind::InvalidData, ""))
+                    })
+                    .transpose()?;
+
+                Poll::Ready(Ok(Some((flag, message))))
+            }
+            None => Poll::Ready(Ok(None)),


Same idea here as mentioned earlier, we could early return from the None branch to reduce some nesting.

thomaseizinger · 2022-09-11T10:54:43Z

+                if !read_buffer.is_empty() {
+                    let n = std::cmp::min(read_buffer.len(), buf.len());


Technically, the if is useless because with a length of 0, all the remaining code is a no-op. Do you think we lose anything in clarify if we make it unconditional?

Actually, that is not true because we would always return Poll::Ready then!

thomaseizinger · 2022-09-11T10:55:17Z

+                }
+            }
+
+            let PollDataChannel { state, io } = &mut *self;


Nit: A question of taste but I'd mildly prefer this.

Suggested change

let PollDataChannel { state, io } = &mut *self;

let Self { state, io } = &mut *self;

thomaseizinger · 2022-09-11T10:55:57Z

+
+            let read_buffer = match state {
+                State::Open {
+                    ref mut read_buffer,


I think borrowing state with &mut state would do the same thing as these keywords.

thomaseizinger · 2022-09-11T11:06:36Z

+}
+
+impl State {
+    fn handle_flag(&mut self, flag: Flag) {


I think this function could benefit from some logging!

Something like "got flag X, moving from state A to B".

Without any kind of connection identifier, this is pretty useless though. The underlying data channel as a "stream identifier'. If we move this function to PollDataChannel, we could access that and include it in the log message.

thomaseizinger · 2022-09-11T11:18:31Z

        buf: &[u8],
    ) -> Poll<io::Result<usize>> {
-        tokio_crate::io::AsyncWrite::poll_write(Pin::new(&mut self.0), cx, buf)
+        // Handle flags iff read side closed.


This definitely needs tests! :)
For example, a really subtle implementation detail is that the check for which state we are in needs to be inside the loop because handling a flag might change to state to one that we no longer want to handle in here.

thomaseizinger · 2022-09-11T11:19:39Z

+enum State {
+    Open { read_buffer: Bytes },
+    WriteClosed { read_buffer: Bytes },
+    ReadClosed { read_buffer: Bytes },
+    ReadWriteClosed { read_buffer: Bytes },
+    ReadReset,
+    ReadResetWriteClosed,
+    Poisoned,
+}
+
+impl State {


Nit: I'd prefer these to be further down in the file because they are implementation details.

thomaseizinger · 2022-09-11T11:21:27Z

-pub struct PollDataChannel(RTCPollDataChannel);
+// TODO
+// #[derive(Debug)]
+pub struct PollDataChannel {


Nit: Can we move away from the PollDataChannel name? IMO it is a weird name from the webrtc library that we don't need to carry over here, plus with this PR, this is doing a lot more than just being a poll-based version of DataChannel / our wrapper around it.

Perhaps this can just be Substream?

thomaseizinger · 2022-09-11T13:13:04Z

+        // TODO: Is flush the correct thing here? We don't want the underlying layer to close both write and read.
+        self.io.poll_flush_unpin(cx).map_err(Into::into)


I think we should have a WriteClosing state that we remain in until we get a Poll::Ready from Sink::poll_flush. That I believe is the contract of Sink we need to uphold.

melekes · 2022-09-19T12:30:21Z

pushed a few commits here: #9

thomaseizinger · 2022-09-19T13:39:20Z

pushed a few commits here: #9

Thank you!

Given that this is a PR against your repo, I think you should have push rights to it. Try to use the GitHub CLI to checkout the PR (gh pr checkout 8) and then put the commits on top of it. This way, we can have the commits in here together with the open comments :)

melekes · 2022-09-20T05:42:43Z

Given that this is a PR against your repo, I think you should have push rights to it. Try to use the GitHub CLI to checkout the PR (gh pr checkout 8) and then put the commits on top of it. This way, we can have the commits in here together with the open comments :)

git push mxinden webrtc-message-framing:webrtc-message-framing
ERROR: Permission to mxinden/rust-libp2p.git denied to melekes.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

looks like I don't have the necessary permissions.

thomaseizinger · 2022-09-20T05:49:31Z

Given that this is a PR against your repo, I think you should have push rights to it. Try to use the GitHub CLI to checkout the PR (gh pr checkout 8) and then put the commits on top of it. This way, we can have the commits in here together with the open comments :)
git push mxinden webrtc-message-framing:webrtc-message-framing
ERROR: Permission to mxinden/rust-libp2p.git denied to melekes.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
looks like I don't have the necessary permissions.

@mxinden You should have a little checkbox on the right next to the "Subscribe" button of the PR that delegates permissions. Can you tick that one please? :)

melekes · 2022-09-20T08:46:33Z

Don't use message framing during noise handshake

why? it would simplify all implementations. the overhead is negligible, I think 🤔

thomaseizinger · 2022-09-20T09:16:45Z

Don't use message framing during noise handshake

why? it would simplify all implementations. the overhead is negligible, I think 🤔

I think that may be an outdated TODO, it is from 9 days ago? I would expect the framing to always happen to simplify the implementation.

melekes · 2022-09-20T11:00:54Z

Send RESET_STREAM when receiving STOP_SENDING

again, not sure why it's needed. The remote has already indicated it won't accept any more data, so why send RESET_STREAM?

mxinden · 2022-09-21T14:20:16Z

Don't use message framing during noise handshake

why? it would simplify all implementations. the overhead is negligible, I think thinking

I think that may be an outdated TODO, it is from 9 days ago? I would expect the framing to always happen to simplify the implementation.

Correct. My comment in the pull request description is outdated. Updated now. Sorry for the trouble.

mxinden · 2022-09-22T09:32:46Z

Send RESET_STREAM when receiving STOP_SENDING

again, not sure why it's needed. The remote has already indicated it won't accept any more data, so why send RESET_STREAM?

Moved to mxinden/specs#1. Hope you don't mind @melekes.

thomaseizinger · 2022-09-22T09:46:57Z

Given that this is a PR against your repo, I think you should have push rights to it. Try to use the GitHub CLI to checkout the PR (gh pr checkout 8) and then put the commits on top of it. This way, we can have the commits in here together with the open comments :)
git push mxinden webrtc-message-framing:webrtc-message-framing
ERROR: Permission to mxinden/rust-libp2p.git denied to melekes.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
looks like I don't have the necessary permissions.
@mxinden You should have a little checkbox on the right next to the "Subscribe" button of the PR that delegates permissions. Can you tick that one please? :)

@mxinden, not sure if you saw this! :)

mxinden · 2022-09-22T16:30:35Z

Thanks for the ping @thomaseizinger. The tick is already set. I am surprised that @melekes can not push here.

mxinden · 2022-09-22T16:56:12Z

Send RESET_STREAM when receiving STOP_SENDING

again, not sure why it's needed. The remote has already indicated it won't accept any more data, so why send RESET_STREAM?

Moved to mxinden/specs#1. Hope you don't mind @melekes.

For the record, this is no longer required. See mxinden/specs#1 (comment). Updated this pull request description.

mxinden · 2022-09-22T16:58:11Z

I am sorry for not giving this pull request the attention it deserves. I don't want this pull request to block libp2p#2622. In case either of you @thomaseizinger or @melekes has capacity, I would appreciate you taking this over. If not, I will do my best spending more time on it.

thomaseizinger · 2022-09-23T05:26:16Z

Thanks for the ping @thomaseizinger. The tick is already set. I am surprised that @melekes can not push here.

Okay, strange. Well lets continue in #9 then.

mxinden · 2022-10-03T18:04:46Z

Closing here in favor of #10.

mxinden added 5 commits August 19, 2022 12:11

transports/webrtc/: Test message framing sizes

59617de

transports/webrtc/: Implement message framing

7dc3ce1

transports/webrtc: Update protobuf

503e32f

Merge remote-tracking branch 'melekes/anton/webrtc-transport' into we…

22e97a0

…brtc-message-framing

transports/webrtc/: Change semantic of RESET

55da918

With mxinden/specs@865f4f2 the RESET no longer resets both write and read part of a stream, but only the former.

mxinden mentioned this pull request Sep 5, 2022

feat: Add WebRTC transport libp2p/rust-libp2p#2622

Merged

4 tasks

thomaseizinger reviewed Sep 8, 2022

View reviewed changes

mxinden added 4 commits September 10, 2022 20:19

transports/webrtc/: Import message_proto types

11c016f

transports/webrtc/: Refactor AsyncRead match arm

1a6e4bd

transports/webrtc/: Handle flags when read side closed

d46a171

transports/webrtc: Enforce maximum message length

9cd4ef7

thomaseizinger reviewed Sep 11, 2022

View reviewed changes

melekes mentioned this pull request Sep 19, 2022

transports/webrtc: Implement stream message framing #9

Closed

9 tasks

mxinden mentioned this pull request Sep 22, 2022

webrtc/: Add message framing to support half-close and reset of stream mxinden/specs#1

Merged

mxinden mentioned this pull request Sep 22, 2022

webrtc/: Add libp2p WebRTC browser-to-server spec libp2p/specs#412

Merged

19 tasks

mxinden closed this Oct 3, 2022

		if !read_buffer.is_empty() {
		let n = std::cmp::min(read_buffer.len(), buf.len());

	let PollDataChannel { state, io } = &mut *self;
	let Self { state, io } = &mut *self;

		// TODO: Is flush the correct thing here? We don't want the underlying layer to close both write and read.
		self.io.poll_flush_unpin(cx).map_err(Into::into)

Conversation

mxinden commented Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Links to any relevant issues

Open Questions

Change checklist

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melekes commented Sep 19, 2022

Uh oh!

thomaseizinger commented Sep 19, 2022

Uh oh!

melekes commented Sep 20, 2022

Uh oh!

thomaseizinger commented Sep 20, 2022

Uh oh!

melekes commented Sep 20, 2022

Uh oh!

thomaseizinger commented Sep 20, 2022

Uh oh!

melekes commented Sep 20, 2022

Uh oh!

mxinden commented Sep 21, 2022

Uh oh!

mxinden commented Sep 5, 2022 •

edited

Loading