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

Respond with ParamOutgoingResetRequest #6

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

algesten
Copy link
Member

This is a fix for the issue reported in algesten/str0m#286

@algesten algesten marked this pull request as draft August 16, 2023 18:55
@algesten
Copy link
Member Author

This is marked as Draft, since I think we need to check spec here. It might be the close calls shouldn't be automatic in sctp-proto, but a stream from outside stream.stop() – looking at pion that might be what we want to do.

@k0nserv is good with the spec detective work.

src/association/mod.rs Outdated Show resolved Hide resolved
@k0nserv
Copy link
Member

k0nserv commented Aug 17, 2023

The spec seems pretty clear: respond with an OutgoingResetRequest when one is received to confirm the closure. However, I'm not sure Pion implemented it like this in the end, there's a lot of relevant discussion in pion/sctp#238

@Marcel-G
Copy link

Marcel-G commented Aug 17, 2023

I tried to replicate pion's fix in webrtc-rs/webrtc which seems to work

@algesten
Copy link
Member Author

So maybe this solve is more correct. If only we could make it close straight away and not wait for the next RTO.

@algesten
Copy link
Member Author

We are not waiting for any RTO. We are sending something straight away, but it seems this gets rejected and then we manage to send the close confirm on retransmit.

@algesten
Copy link
Member Author

I think this might be ready. The problem was that the response ParamOutgoingResetRequest needs to have the reconfig_request_sequence_number set. And now it does. The channels close promptly.

@algesten algesten marked this pull request as ready for review August 17, 2023 17:15
@algesten algesten requested a review from rainliu August 17, 2023 17:15
@algesten
Copy link
Member Author

Hi @rainliu what do you think about this fix?

@rainliu rainliu merged commit a8edab0 into webrtc-rs:main Aug 24, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants