Ensure publisher connection is reconnected if needed #874
Conversation
|
size-limit report 📦
|
| this.subscriberPrimary = joinResponse.subscriberPrimary; | ||
| if (!this.publisher) { | ||
| this.configure(joinResponse); | ||
| } | ||
|
|
||
| // create offer | ||
| if (!this.subscriberPrimary) { | ||
| if (!this.subscriberPrimary || this.hasPublished) { |
There was a problem hiding this comment.
this is needed to re-establish publisher connection on full reconnects
| @@ -1092,11 +1105,10 @@ export default class RTCEngine extends (EventEmitter as new () => TypedEventEmit | |||
| // this means we'd have to check its status manually and update address | |||
| // manually | |||
| now - startTime > minReconnectWait && | |||
| this.primaryPC?.connectionState === 'connected' | |||
| this.primaryPC?.connectionState === 'connected' && | |||
| (!this.hasPublished || this.publisher?.pc.connectionState === 'connected') | |||
There was a problem hiding this comment.
we only set it to connected if - in case we need it - the publisher connection is also connected
| if (shouldEmit) { | ||
| const initialFullConnection = | ||
| (this.pcState === PCState.New && !this.hasPublished) || | ||
| this.publisher?.pc.connectionState === 'connected'; |
There was a problem hiding this comment.
I'm not sure if this is right.. if publisher is connected before the subscriber in the reconnect case.. it should not emit this event.
I think the idea is that this would only emit event in case it's the initial connection, and reconnect would be checking its state/events in either the resume or full reconnect paths.
There was a problem hiding this comment.
what is this change attempting to fix? it seems to increase complexity as we try to coordinate both peer connection states.
There was a problem hiding this comment.
you're right, we'd have to make sure not to emit this in the reconnect case.
the idea is to have a single understanding of what a connected state is. If subscriberPrimary but we also publish something, the connected state only happens if both peer connections are connected
| this.setAndEmitConnectionState(ConnectionState.Connected); | ||
| this.emit(RoomEvent.Reconnected); | ||
| this.registerConnectionReconcile(); | ||
| // only set as connected if _only_ the signal connection had been severed |
There was a problem hiding this comment.
not understanding the comment. it seems to be flipping it back only when PC transport have been restored.
There was a problem hiding this comment.
maybe bad wording.
At this point we would only want to emit Reconnected if the transports still work. In case the transport don't we would wait for them to re-establish until a Reconnected event is fired.
|
closing in favor of #875 |
No description provided.