fix(quic): allow closing stream after remote dropped it #3164

elenaf9 · 2022-11-24T17:03:08Z

Description

Only send STOP_SENDING on a stream when dropping it if the remote did not finish the stream yet.
Only call quinn_proto::SendStream::finish once. (A second call to it will always fail. Though I don't think this was the issue in transports/quic: STOP_SENDING fails poll_close #3144.)
Add tests for reading and writing to streams after the remote dropped. Also adds a smoke test for backpressure.

Links to any relevant issues

Fixes #3144 following proposed idea nr.2 from @mxinden.

Don't send a STOP_SENDING on DROP IFF the remote already closed the write side.

That would be in-line with what upstream quinn does. https://github.com/quinn-rs/quinn/blob/1d390af2facdf424c7feb470909fffc29a1dca6c/quinn/src/recv_stream.rs#L396-L400

I implemented it slightly different compared to quinn, since quinn only sets the all_data_read flag when the stream finishes while reading. In identify however we might receive a FIN for a stream on which we are not reading anymore, thus what we do now is in the drop implementation we manually check the receive stream again if it has finished.

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

- Don't call `quinn_proto::SendStream::finish` if the stream is already closing. - Only stop a stream on drop if the remote did not finish it yet.

mxinden

Thanks @elenaf9. I will test this out in a bit.

transports/quic/src/connection/substream.rs

mxinden · 2022-11-24T18:21:52Z

I deployed this to kademlia-exporter.max-inden.de/ and successfully connected to it via a patched https://github.com/mxinden/libp2p-lookup.

elenaf9 · 2022-11-24T18:45:28Z

I deployed this to kademlia-exporter.max-inden.de/ and successfully connected to it via a patched https://github.com/mxinden/libp2p-lookup.

Nice! Do I understand it correctly that before it always failed due to the issue in identify? Or was it a flaky problem and we need to do some more testing before we can confirm #3144 is fixed?

mxinden · 2022-11-24T19:23:45Z

Nice! Do I understand it correctly that before it always failed due to the issue in identify?

Correct. It was consistently failing.

mxinden

Small suggestion. Otherwise looks good to me. Thanks for the patch and the unit tests!

mxinden · 2022-11-24T19:36:00Z

transports/quic/src/connection/substream.rs

-    /// `true` if the stream finished, i.e. the writing side closed.
+    /// `true` if the writing side of the stream is closing, i.e. `AsyncWrite::poll_close`
+    /// was called.
+    pub is_finishing: bool,


Suggested change

pub is_finishing: bool,

pub is_write_closing: bool,

Would that not be more intuitive / consistent with is_write_closed?

An alternative would be to introduce an enum:

write_state: WriteState, // ... enum WriteState { Open, Closing, Closed, }

I don't have a strong opinion.

finish is QUIC terminology right?

I like WriteState. Added in c6256e1.

finish is QUIC terminology right?

Not really QUIC terminology, but used in quinn. I stuck with it to avoid confusion on inner events like StreamEvent::Finished. But thinking about it again I think using the AsyncWrite terminology of close is more intuitive.

mxinden · 2022-11-24T19:42:47Z

transports/quic/tests/smoke.rs

+    let too_much_data = vec![0; 10];
+
+    assert!(
+        async_std::future::timeout(Duration::from_secs(1), stream_a.write_all(&too_much_data))


Usage of a timeout in a unit test is unfortunate. That said, I can not think of an alternative, and I think it is worth it. Thus I suggest let's keep as is.

You could assert that it returns Poll::Pending instead and Poll::Ready once you read from the other side again.

You could assert that it returns Poll::Pending instead and Poll::Ready once you read from the other side again.

👍. Done in 9f2a7ee.

mxinden · 2022-11-24T19:43:48Z

transports/quic/tests/smoke.rs

@@ -182,6 +183,66 @@ fn concurrent_connections_and_streams_tokio() {
        .quickcheck(prop::<quic::tokio::Provider> as fn(_, _) -> _);
 }

+#[cfg(feature = "async-std")]
+#[async_std::test]
+async fn backpressure() {


thomaseizinger

Nice! A few suggestions!

thomaseizinger · 2022-11-24T20:20:11Z

transports/quic/tests/smoke.rs

+#[cfg(feature = "async-std")]
+#[async_std::test]
+async fn read_after_peer_dropped_stream() {
+    let _ = env_logger::try_init();
+    let (mut stream_a, mut stream_b) = build_streams::<quic::async_std::Provider>().await;
+
+    let data = vec![0; 10];
+
+    stream_b.close().now_or_never();
+    stream_a.write_all(&data).await.unwrap();
+    stream_a.close().await.unwrap();
+    drop(stream_a);
+
+    stream_b.close().await.unwrap();
+    let mut buf = Vec::new();
+    stream_b.read_to_end(&mut buf).await.unwrap();
+    assert_eq!(data, buf)
+}
+
+#[cfg(feature = "async-std")]
+#[async_std::test]
+#[should_panic]
+async fn write_after_peer_dropped_stream() {
+    let _ = env_logger::try_init();
+    let (stream_a, mut stream_b) = build_streams::<quic::async_std::Provider>().await;
+    drop(stream_a);
+    futures_timer::Delay::new(Duration::from_millis(1)).await;
+
+    let data = vec![0; 10];
+    stream_b.write_all(&data).await.expect("Write failed.");
+    stream_b.close().await.expect("Close failed.");
+}


Can we move these to the muxer test harness? :)

I am in favor of moving them to the test harness.
However, contrary to the other tests in the harness these tests test a specific sequential order in which alice and bob do their calls, rather than the things running in parallel. So I think it would require some more "infrastructure" code and ideally we'd also add it for the other muxers. Since I don't want to block this PR on this I would propose to do it in a follow up PR.

Right, that makes sense. I didn't consider that :)

Pushing the boundaries of the harness here 😁

Tracked the idea in issue #3211.

thomaseizinger · 2022-11-24T20:23:22Z

transports/quic/tests/smoke.rs

+    let too_much_data = vec![0; 10];
+
+    assert!(
+        async_std::future::timeout(Duration::from_secs(1), stream_a.write_all(&too_much_data))


You could assert that it returns Poll::Pending instead and Poll::Ready once you read from the other side again.

thomaseizinger · 2022-11-24T20:24:51Z

transports/quic/tests/smoke.rs

+
+    let data = vec![0; 10];
+    stream_b.write_all(&data).await.expect("Write failed.");
+    stream_b.close().await.expect("Close failed.");


Is this the line that will panic? I'd like to see an unwrap_err then.

Unfortunately this is not deterministic; it may either panic on the write_all or on the close, depending on whether the STOP_SENDING frame made it to B before B starts writing or not.

thomaseizinger · 2022-11-24T20:26:06Z

transports/quic/src/connection/substream.rs

-    /// `true` if the stream finished, i.e. the writing side closed.
+    /// `true` if the writing side of the stream is closing, i.e. `AsyncWrite::poll_close`
+    /// was called.
+    pub is_finishing: bool,


finish is QUIC terminology right?

thomaseizinger · 2022-11-24T20:40:48Z

A few suggestions regarding the PR description/title:

I'd suggest for scope to not be paths but modules, i.e. just quic is good enough and shortens the title (under 50 characters total is a good convention for commit messages)
"fix" is currently repeated, maybe a better title would be: "fix(quic): allow to read from a closed & dropped stream". i.e. describe what is possible now but failed before
Instead of bullet points in the commit message body (i.e. PR description), make it one or more paragraphs.
Put the Fixes #XYZ as the last line of the commit message.

I also often reference this blog post: https://cbea.ms/git-commit/

If we follow these guidelines, all squash commits will be nice commits ☺️

elenaf9 · 2022-11-25T01:05:17Z

A few suggestions regarding the PR description/title:

I'd suggest for scope to not be paths but modules, i.e. just quic is good enough and shortens the title (under 50 characters total is a good convention for commit messages)

"fix" is currently repeated, maybe a better title would be: "fix(quic): allow to read from a closed & dropped stream". i.e. describe what is possible now but failed before

Instead of bullet points in the commit message body (i.e. PR description), make it one or more paragraphs.

Put the Fixes #XYZ as the last line of the commit message.

I also often reference this blog post: https://cbea.ms/git-commit/

If we follow these guidelines, all squash commits will be nice commits relaxed

Agree with all but nr 3. I personally prefer bullet points to avoid unnecessary filler words, at least if no additional explanation is needed 🙂

Edit: Ah you might be referring to the formatting and not the phrasing itself. I don't feel strongly about it, but why not use bullet points?

mxinden · 2022-11-25T08:32:25Z

Adding send-it here to include it in libp2p v0.50.0. As far as I can tell, only outstanding discussion is the move to the muxer test harness (#3164 (comment)). @elenaf9 could you follow up on that?

Thanks for prioritizing this @elenaf9 🙏

thomaseizinger · 2022-11-25T20:18:29Z

A few suggestions regarding the PR description/title:

I'd suggest for scope to not be paths but modules, i.e. just quic is good enough and shortens the title (under 50 characters total is a good convention for commit messages)

"fix" is currently repeated, maybe a better title would be: "fix(quic): allow to read from a closed & dropped stream". i.e. describe what is possible now but failed before

Instead of bullet points in the commit message body (i.e. PR description), make it one or more paragraphs.

Put the Fixes #XYZ as the last line of the commit message.

I also often reference this blog post: https://cbea.ms/git-commit/

If we follow these guidelines, all squash commits will be nice commits relaxed

Agree with all but nr 3. I personally prefer bullet points to avoid unnecessary filler words, at least if no additional explanation is needed 🙂

Edit: Ah you might be referring to the formatting and not the phrasing itself. I don't feel strongly about it, but why not use bullet points?

I totally get what you are saying and I agree with the gist of it. The issue with bullet points can be that they are often too concise. It is sometimes nice to read a little paragraph that summarizes what is happening and why. Often, I start the commit message with:

"Previously, our behaviour was XYZ. That caused problem ABC. We fix this by doing 1, 2 and 3."

That could or could not involve bulletpoints but it makes it clear what is the description of the original behaviour, what is the problem and what is the fix. I don't think there are too many unnecessary filler words in there but YMMV :)

Here is a recent example: #3152

I don't feel too strongly about it, just noticed it and wanted to start a discussion to see where everyone is at!

elenaf9 added 2 commits November 24, 2022 18:01

tests(quic): add stream tests

019adf0

fix(quic): fix closing and dropping of streams

8bda2c7

- Don't call `quinn_proto::SendStream::finish` if the stream is already closing. - Only stop a stream on drop if the remote did not finish it yet.

mxinden reviewed Nov 24, 2022

View reviewed changes

transports/quic/src/connection/substream.rs Outdated Show resolved Hide resolved

mxinden reviewed Nov 24, 2022

View reviewed changes

thomaseizinger approved these changes Nov 24, 2022

View reviewed changes

elenaf9 added 2 commits November 25, 2022 01:26

refactor(quic): Add WriteState for substreams

c6256e1

test(quic): avoid using timeout in unit test

9f2a7ee

elenaf9 changed the title ~~fix(transports/quic): fix closing stream after remote dropped it~~ fix(quic): allow closing stream after remote dropped it Nov 25, 2022

elenaf9 added 2 commits November 25, 2022 02:05

Merge branch 'master' into quic/fix-substream-close

163b2c1

doc(quic): fix intra-doc link

a75e492

mxinden approved these changes Nov 25, 2022

View reviewed changes

mxinden added the send-it label Nov 25, 2022

Merge branch 'master' into quic/fix-substream-close

4e7c025

mergify bot merged commit 7b9611e into libp2p:master Nov 25, 2022

elenaf9 mentioned this pull request Dec 8, 2022

Move quic/tests/smoke::{read, write}_after_peer_dropped_stream to muxer test harness #3211

Open

mxinden mentioned this pull request Oct 29, 2023

fix(hole-punch): don't explicitly close RESERVE stream #4747

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(quic): allow closing stream after remote dropped it #3164

fix(quic): allow closing stream after remote dropped it #3164

elenaf9 commented Nov 24, 2022 •

edited

Loading

mxinden left a comment

mxinden commented Nov 24, 2022

elenaf9 commented Nov 24, 2022

mxinden commented Nov 24, 2022

mxinden left a comment

mxinden Nov 24, 2022

mxinden Nov 24, 2022

thomaseizinger Nov 24, 2022

elenaf9 Nov 25, 2022

mxinden Nov 24, 2022

thomaseizinger Nov 24, 2022

elenaf9 Nov 25, 2022

mxinden Nov 24, 2022

thomaseizinger left a comment

thomaseizinger Nov 24, 2022

elenaf9 Nov 24, 2022

thomaseizinger Nov 25, 2022

elenaf9 Dec 8, 2022

thomaseizinger Nov 24, 2022

thomaseizinger Nov 24, 2022

elenaf9 Nov 25, 2022

thomaseizinger Nov 24, 2022

thomaseizinger commented Nov 24, 2022

elenaf9 commented Nov 25, 2022 •

edited

Loading

mxinden commented Nov 25, 2022

thomaseizinger commented Nov 25, 2022 •

edited

Loading

fix(quic): allow closing stream after remote dropped it #3164

fix(quic): allow closing stream after remote dropped it #3164

Conversation

elenaf9 commented Nov 24, 2022 • edited Loading

Description

Links to any relevant issues

Change checklist

mxinden left a comment

Choose a reason for hiding this comment

mxinden commented Nov 24, 2022

elenaf9 commented Nov 24, 2022

mxinden commented Nov 24, 2022

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger commented Nov 24, 2022

elenaf9 commented Nov 25, 2022 • edited Loading

mxinden commented Nov 25, 2022

thomaseizinger commented Nov 25, 2022 • edited Loading

elenaf9 commented Nov 24, 2022 •

edited

Loading

elenaf9 commented Nov 25, 2022 •

edited

Loading

thomaseizinger commented Nov 25, 2022 •

edited

Loading