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

Properly handle data channel opening failures #2936

Merged
merged 4 commits into from
Oct 27, 2022
Merged

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Oct 27, 2022

Fix #2900

In order to open outbound substreams on a WebRTC connection, the mechanism that the API of smoldot uses is as follows: one function to ask the implementation to open a substream, and one callback that is later called in order to notify that a substream has been fully opened.

The API of smoldot doesn't provide a way to report that opening an outbound substream has failed. This is because this isn't really supposed to happen. If you have a connection, opening a substream is purely a "local side change": you allocate an ID, then send a message. It should maybe not even be asynchronous, but that's off-topic here.

The problem here is that this mental model doesn't really fit what the browser does. In particular, the browser does nothing until a data channel is first opened, plus the browser can call onclose or onerror on the data channel before onopen. To accomodate for that, the current code is a bit hacky in the sense that it reports the connection as immediately open.

This PR does it a bit more differently and more properly: we immediately open a data channel at the same time as we create the RTCPeerConnection in order for the browser to start connecting, even though smoldot didn't request a channel yet. We store this first data channel on the side, and when smoldot asks for a data channel we provide this one.
If a data channel closes while it is still opening (the handshake one or another), we just kill the entire connection.

@tomaka tomaka requested a review from melekes October 27, 2022 12:37
Copy link
Contributor

@mergify mergify bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automatically approving tomaka's pull requests. This auto-approval will be removed once more maintainers are active.

@tomaka
Copy link
Contributor Author

tomaka commented Oct 27, 2022

I have a headache from the complexity brought by the fact that there are dozens and dozens of asynchronous functions and that things can be called in many different orders. But I think that such is the nature JS and there's no great way to make the code more easy.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2022

twiggy diff report

Difference in .wasm size before and after this pull request.


 Delta Bytes │ Item
─────────────┼──────────────────
          +0 ┊ Σ [0 Total Rows]

@tomaka tomaka added the automerge Automatically merge pull request as soon as possible label Oct 27, 2022
@mergify mergify bot merged commit ee72a59 into paritytech:main Oct 27, 2022
@tomaka tomaka deleted the fix-2900 branch October 27, 2022 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge pull request as soon as possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WebRTC opening failure panic
2 participants