Reset outgoing stream on inbound is reset #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Added state to Stream to reset outbound only when the state is open Relates to #187
enobufs
commented
Oct 2, 2022
| }, | ||
| c, | ||
| }) | ||
|
|
There was a problem hiding this comment.
Here, I am simply reverting the change.
enobufs
commented
Oct 2, 2022
| @@ -2014,6 +2014,9 @@ func (a *Association) resetStreamsIfAny(p *paramOutgoingResetRequest) *packet { | |||
| if !ok { | |||
| continue | |||
| } | |||
| a.lock.Unlock() | |||
| s.onInboundStreamReset() | |||
| a.lock.Lock() | |||
There was a problem hiding this comment.
Reporting the event to the corresponding stream and let it decide what to do.
enobufs
commented
Oct 2, 2022
| } | ||
| s.lock.Unlock() | ||
| default: | ||
| } |
There was a problem hiding this comment.
This state checks are done inside sendPayloadData()
enobufs
commented
Oct 2, 2022
| if err != nil { | ||
| return 0, err | ||
| return n, errStreamClosed |
There was a problem hiding this comment.
Try to use the same error object to be compatible.
enobufs
commented
Oct 2, 2022
|
|
||
| if s.readErr == nil { | ||
| s.readErr = io.EOF |
There was a problem hiding this comment.
This is not what RFC 8831 implies. s.Close() should only close the outgoing stream. (fixing the existing bug)
enobufs
commented
Oct 2, 2022
| if resetOutbound { | ||
| s.association.sendResetRequest(sid) | ||
| } | ||
| } |
There was a problem hiding this comment.
Finally, this is what we wanted in the PR.
|
@enobufs Sorry for the delay, just come back form vacation. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added state to Stream to reset outbound only when the state is open Relates to pion#187
Hi @jerry-tao, what do you think about this fix. (this branch is based on your fork
jerry-tao/sctp:master).Who to react to the incoming OutgoingResetRequest should be Stream layer (not at the Association layer).
Also, if the stream already sent its OutgoingResetRequest, it shouldn't send another one, meaning, we need to introduce a state in the Stream class (s.state), and send OutgoingResetRequest if the state is "open", I believe. Which prevented the vnet test case from failing.
As RFC 8831 implies, teardown sequence of a stream starts with outgoing (closing), then incoming (closed). This branch also addresses it by not to cause error (s.readErr) because it will eventually be reset by the remote.
I am hoping to merge this into your branch, then we can finalize it in your pull-request.