Skip to content

Commit

Permalink
fix: delay notification of early WebRTC stream creation (#2206)
Browse files Browse the repository at this point in the history
The datachannel muxer is created during set up of the `Connection` object.  If we notify of early stream creation before the `Connection` object is properly configured, the early streams will be lost.

This can happen when the remote opens a data channel before the local node has finished setting up it's end of the connection.

The fix is to notify asynchronously which gives the upgrader enough time to finish setting up the `Connection`.
  • Loading branch information
achingbrain authored Nov 6, 2023
1 parent dfbe0cc commit d25d951
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 13 deletions.
26 changes: 18 additions & 8 deletions packages/transport-webrtc/src/muxer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,14 +137,24 @@ export class DataChannelMuxer implements StreamMuxer {
init?.onIncomingStream?.(stream)
}

this.init.streams.forEach(bufferedStream => {
bufferedStream.onEnd = () => {
this.#onStreamEnd(bufferedStream.stream, bufferedStream.channel)
}

this.metrics?.increment({ incoming_stream: true })
this.init?.onIncomingStream?.(bufferedStream.stream)
})
// the DataChannelMuxer constructor is called during set up of the
// connection by the upgrader.
//
// If we invoke `init.onIncomingStream` immediately, the connection object
// will not be set up yet so add a tiny delay before letting the
// connection know about early streams
if (this.init.streams.length > 0) {
queueMicrotask(() => {
this.init.streams.forEach(bufferedStream => {
bufferedStream.onEnd = () => {
this.#onStreamEnd(bufferedStream.stream, bufferedStream.channel)
}

this.metrics?.increment({ incoming_stream: true })
this.init?.onIncomingStream?.(bufferedStream.stream)
})
})
}
}

#onStreamEnd (stream: Stream, channel: RTCDataChannel): void {
Expand Down
15 changes: 10 additions & 5 deletions packages/transport-webrtc/src/private-to-private/transport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ export class WebRTCTransport implements Transport, Startable {
log.trace('dialing address: %a', ma)

const peerConnection = new RTCPeerConnection(this.init.rtcConfiguration)
const muxerFactory = new DataChannelMuxerFactory({
peerConnection,
dataChannelOptions: this.init.dataChannel
})

const { remoteAddress } = await initiateConnection({
peerConnection,
Expand All @@ -141,10 +145,7 @@ export class WebRTCTransport implements Transport, Startable {
const connection = await options.upgrader.upgradeOutbound(webRTCConn, {
skipProtection: true,
skipEncryption: true,
muxerFactory: new DataChannelMuxerFactory({
peerConnection,
dataChannelOptions: this.init.dataChannel
})
muxerFactory
})

// close the connection on shut down
Expand All @@ -156,6 +157,10 @@ export class WebRTCTransport implements Transport, Startable {
async _onProtocol ({ connection, stream }: IncomingStreamData): Promise<void> {
const signal = AbortSignal.timeout(this.init.inboundConnectionTimeout ?? INBOUND_CONNECTION_TIMEOUT)
const peerConnection = new RTCPeerConnection(this.init.rtcConfiguration)
const muxerFactory = new DataChannelMuxerFactory({
peerConnection,
dataChannelOptions: this.init.dataChannel
})

try {
const { remoteAddress } = await handleIncomingStream({
Expand All @@ -178,7 +183,7 @@ export class WebRTCTransport implements Transport, Startable {
await this.components.upgrader.upgradeInbound(webRTCConn, {
skipEncryption: true,
skipProtection: true,
muxerFactory: new DataChannelMuxerFactory({ peerConnection, dataChannelOptions: this.init.dataChannel })
muxerFactory
})

// close the stream if SDP messages have been exchanged successfully
Expand Down
44 changes: 44 additions & 0 deletions packages/transport-webrtc/test/muxer.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/* eslint-disable @typescript-eslint/no-unused-expressions */

import { expect } from 'aegir/chai'
import pRetry from 'p-retry'
import { stubInterface } from 'sinon-ts'
import { DataChannelMuxerFactory } from '../src/muxer.js'

describe('muxer', () => {
it('should delay notification of early streams', async () => {
let onIncomingStreamInvoked = false

// @ts-expect-error incomplete implementation
const peerConnection: RTCPeerConnection = {}

const muxerFactory = new DataChannelMuxerFactory({
peerConnection
})

// simulate early connection
// @ts-expect-error incomplete implementation
const event: RTCDataChannelEvent = {
channel: stubInterface<RTCDataChannel>({
readyState: 'connecting'
})
}
peerConnection.ondatachannel?.(event)

muxerFactory.createStreamMuxer({
onIncomingStream: () => {
onIncomingStreamInvoked = true
}
})

expect(onIncomingStreamInvoked).to.be.false()

await pRetry(() => {
if (!onIncomingStreamInvoked) {
throw new Error('onIncomingStreamInvoked was still false')
}
})

expect(onIncomingStreamInvoked).to.be.true()
})
})

0 comments on commit d25d951

Please sign in to comment.