Clear peers on disconnect #168
Conversation
src/classes/HiFiMixerSession.ts
Outdated
| } | ||
| // TODO: remove the entry from the peer state cache -- is this OK? | ||
| //delete this._mixerPeerKeyToStateCacheDict[mixerPeerKey]; | ||
| delete this._mixerPeerKeyToStateCacheDict[key]; |
There was a problem hiding this comment.
While looking over the code I noticed this for loop was performing double map lookups in _mixerPeerKeyToStateCacheDict in the common case and triple lookups in the uncommon. This offends my sensibilities for high performance code and so I was compelled to fix it.
There was a problem hiding this comment.
Oh yeah, also notice that I did uncomment the code for the old TODO. Yes, it is a good idea to actually purge old peer data from the cache when it gets deleted.
| @@ -699,6 +699,7 @@ export class HiFiMixerSession { | |||
| await close(this._raviSignalingConnection, "Signaling Connection", RaviSignalingStates.CLOSED); | |||
| await close(this._raviSession, "Session", RaviSessionStates.CLOSED); | |||
|
|
|||
| this._clearPeerData(); | |||
There was a problem hiding this comment.
Here is the real fix for HIFI-847: clear the peer cache on disconnect.
| allDeletedUserData.push(deletedUserData); | ||
| } | ||
| this.onUsersDisconnected(allDeletedUserData); | ||
| } | ||
| this._mixerPeerKeyToStateCacheDict = {}; | ||
| } | ||
| } |
There was a problem hiding this comment.
Note: we aren't just clearing the peer cache here. If onUserDisconnect callback exists we also assemble allDeletedUserData and feed it to onUserDisconnect. This tells the Application to remove any stale peer data it happens to be using.
| this.onUsersDisconnected(allDeletedUserData); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I've moved the handling of deleted_visit_ids down below the handling of peers. It is theoretically possible for a change to exist in peers for someone who was also deleted in the same frame, and when a change is not found in the map the peer is added to it. By handling the deletions last we can avoid re-adding the changed user after deletion.
| if (peerData.providedUserID) { | ||
| deletedUserData.providedUserID = peerData.providedUserID; | ||
| } | ||
| delete this._mixerPeerKeyToStateCacheDict[key]; |
There was a problem hiding this comment.
Note: I'm correctly deleting the peer from the map on delete. Previously this operation was commented out with a TODO comment about it (see the above removed block).
This PR fixes bug HIFI-847 in the JS API.
https://highfidelity.atlassian.net/browse/HIFI-847
This work is a theoretical fix: I haven't been able to test it yet.