fix: WebRTC uncaught promise rejection on incoming connection #2302
Conversation
We catch WebRTC errors, log them and throw normalised errors to ensure they are the same across WebRTC stacks (FF, Chrome, node). if thrown.
| log.trace('recipient connected, closing signaling stream') | ||
| await messageStream.unwrap().unwrap().close({ | ||
| signal | ||
| }) |
There was a problem hiding this comment.
This method is called from _onProtocol in transport.ts which handles closing/aborting the stream so there's no need to do it here.
| @@ -20,8 +19,6 @@ export async function handleIncomingStream ({ peerConnection, stream, signal, co | |||
| const messageStream = pbStream(stream).pb(Message) | |||
|
|
|||
| try { | |||
| const answerSentPromise: DeferredPromise<void> = pDefer() | |||
There was a problem hiding this comment.
No need for this, we can just await the promises returned by the WebRTC stack.
| @@ -96,11 +87,6 @@ export async function handleIncomingStream ({ peerConnection, stream, signal, co | |||
| signal, | |||
| log | |||
| }) | |||
There was a problem hiding this comment.
should we still log.trace('recipient connected, closing signaling stream') ?
There was a problem hiding this comment.
I would have thought not, since this function isn't closing the signalling stream any more.
There was a problem hiding this comment.
I was thinking that we're still waiting for readCandidatesUntilConnected and the last log was "reading until connected", so in logs it could look like you're never done reading, nor connected, unless we see a "done reading" message
maybe my original comment should have asked for keeping log.trace('recipient connected, stopped reading')
| peerConnection.setLocalDescription(answer).then(() => { | ||
| answerSentPromise.resolve() | ||
| }, err => { | ||
| await peerConnection.setLocalDescription(answer).catch(err => { |
We catch WebRTC errors, log them and throw normalised errors to ensure they are the same across WebRTC stacks (FF, Chrome, node).
#2299 made one of these operations async which would cause an UHPR if thrown.
Change checklist