fix(s2n-quic-transport): close all streams after Endpoint drop (#3170) - #3171
fix(s2n-quic-transport): close all streams after Endpoint drop (#3170)#3171SanjoDeundiak wants to merge 1 commit into
Conversation
4c486a6 to
d57ae59
Compare
| use smallvec::SmallVec; | ||
|
|
||
| // There is no list with error codes, so let's use a value we like | ||
| const ENDPOINT_DROPPED_ERROR_CODE: VarInt = VarInt::from_u8(42); |
There was a problem hiding this comment.
I don't think this is the right decision. Choosing a random error code sounds wrong in this scenario. I think it would be better if we can close the stream with a proper existing error code. I think you should look into connection::Error and find an appropriate one.
There was a problem hiding this comment.
I agree that this error would be more appropriate, however application_close expects application::Error error type, so public API should change to make that happen. I chose to change the type to connection::Error for the application_close argument, since it's converted to that type anyways, but let me know if you have something different in mind
) Currently, after dropping the Endpoint the following is observed: - tx operations succeed - rx operations never return from an .await This is concerning because Endpoint drop can happen particularly due to IO implementation returning an error, which makes the current behaviour unexpected and hard to handle. The expected behaviour is: - tx operations return an error - rx operations return an error
d57ae59 to
8f63263
Compare
|
@boquan-fang thanks for the review! Could you please take another look? |
|
Hi @SanjoDeundiak, Thanks for contributing! I will give your PR another review later on. In the meanwhile, can you work to fix those errors that this PR caused on the CI? Thanks! |
2 participants
Release Summary:
Close all streams after
EndpointdropResolved issues:
#3170
Description of changes:
Currently, after dropping the Endpoint the following is observed:
This is concerning because Endpoint drop can happen particularly
due to IO implementation returning an error, which makes the current
behaviour unexpected and hard to handle.
The expected behaviour is:
To achieve this behaviour I suggest closing all streams on Endpoint drop
Testing:
I added tests and made sure they do not pass before the change and pass after the change
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.