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

QUIC: handle the case of both peers half-closing a stream #3343

Closed
elenaf9 opened this issue Jan 17, 2023 · 4 comments
Closed

QUIC: handle the case of both peers half-closing a stream #3343

elenaf9 opened this issue Jan 17, 2023 · 4 comments
Labels
difficulty:moderate priority:important The changes needed are critical for libp2p, or are blocking another project

Comments

@elenaf9
Copy link
Contributor

elenaf9 commented Jan 17, 2023

Summary

If for a QUIC stream both peers agree at the same time that no more data should be send in one direction, the one side tries to close the stream (send a FIN), while the other sends a STOP_SENDING.

Receiving a STOP_SENDING before / while closing a stream right now causes the close call to return an Error::Stopped.
However, in practice in the case that all of our write data was received and acknowledged by the remote, this should not be an error. Instead both peers simply reached the agreement that the stream is closed in this direction, and no data was lost.

The current behaviour is causing issues like #3298 and #3281.

Expected behaviour

If all write data has been received (and ack'ed) by the remote peer closing a stream should not fail.

Actual behaviour

Substream::close always returns an error if the stream is stopped by the peer before the FIN frame is acknowledged.

Possible Solution

We have been discussion different solutions for this out of band in #3302.
One option would be to simply treat a stopped send-stream as successfully closed. Typically a remote would only send a STOP_SENDING if it read all of our data.
However, if something unexpected happened and the remote stopped the stream (e.g. because it dropped the stream) before reading our latest data, the error would be lost. Our user would in this case wrongly assume that the remote read the data, when it actually didn't.

Another option would be to check if we still have unacknowledged data on the stream, and depending on that return Ok/Err. The quinn-proto API currently doesn't give us this information, so we'd have to add an upstream patch.
Side note: the high-level quinn implementation handles this case the same way as we do it, i.e. return an Error in the close call if the stream stopped. Given that we long-term consider switching to quinn, this is also something we should take into consideration.

I still need to think more about this. Input is appreciated.
There are easy fixes for #3298 #3281 on the application layer for which I'll do PRs, but long-term we should fix this within libp2p-quic.

// cc @marten-seemann

Version

  • libp2p-quic 0.7.0-alpha.2

Would you like to work on fixing this bug?

Yes

@thomaseizinger
Copy link
Contributor

Another option would be to check if we still have unacknowledged data on the stream, and depending on that return Ok/Err. The quinn-proto API currently doesn't give us this information, so we'd have to add an upstream patch.

I think this is the way to go.

Side note: the high-level quinn implementation handles this case the same way as we do it, i.e. return an Error in the close call if the stream stopped.

I'd consider this a bug. It should be possible as a protocol designer to use "closing the read side" as a "I have read all data I was expecting". If the other side at the same time closes the write end, both are in agreement on the protocol semantics and we should continue on the happy path.

Can you open an issue with quinn?

@elenaf9
Copy link
Contributor Author

elenaf9 commented Jan 29, 2023

Can you open an issue with quinn?

Opened quinn-rs/quinn#1487.

@elenaf9
Copy link
Contributor Author

elenaf9 commented Feb 9, 2023

I thought about this some more after discussions with @marten-seemann, @mxinden and quinn-rs/quinn#1487.
We can never be sure if the remote actually received the data that we sent. Even if the QUIC stack acknowledged all packets, we don't know if the application read the data. And at the same time, it could be that the remote read all data, but the Ack was simply lost / the STOP_SENDING received before the Ack. In that case we'd have a false error.
So even with this fix we can never assume anything on the application layer.

Maybe it's the best fix to simply not error on a STOP_SENDING after all.
Alternatively / additionally, we might also want to reconsider if the close operation should even wait for the FIN to be acknowledged (like it is currently done, and also in quinn), or simply directly return without caring about the remote acknowledging the FIN. That would be more consistent with how it is done in Yamux.
How is it done in WebRTC @melekes @thomaseizinger?

Side note: I won't be tackling this in the near future. If anybody could take ownership of this issue, it would be much appreciated!

mergify bot pushed a commit that referenced this issue Feb 19, 2023
Don't close the stream `protocol::recv`.

This is a short-term fix for #3298.

The issue behind this is a general one on the QUIC transport when closing streams, as described in #3343. This PR only circumvents the issue for identify. A proper solution for our QUIC transport still needs more thought.

Pull-Request: #3344.
@thomaseizinger
Copy link
Contributor

This is likely stale now that we use quinn directly.

@thomaseizinger thomaseizinger closed this as not planned Won't fix, can't repro, duplicate, stale Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty:moderate priority:important The changes needed are critical for libp2p, or are blocking another project
Projects
None yet
Development

No branches or pull requests

2 participants