Repository navigation
fix(js-streamer): clean up player connections - #1128
Conversation
🦋 Changeset detectedLatest commit: 818fda5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Thanks for picking this up, and for the thorough test plan. Switching to the Map API plus a single removePlayer() path looks right to me. Two small requests before we merge:
In streamer_shutdown.spec.ts, the first test also checks that the data protocol and initial settings are delivered (the messages handle and the sendDataProtocol / sendInitialSettings round-trip, roughly lines 75–108). That isn't about cleanup, and it ends up depending on an existing bug: sendDataProtocol's second loop writes into streamerProto instead of playerProto, so the Direction: 1 message is basically empty. That's why the assertion can only use objectContaining({ Direction: 1 }). If someone fixes that later, a test about shutdown would break for an unrelated reason. Could you remove that block so the test only covers shutdown? If you'd like to keep the protocol coverage, a separate test is fine. Better still, fix the playerProto loop in a follow-up PR and assert the full message there.
Could you rename the PR to fix(js-streamer): clean up player connections? That matches how we scope titles elsewhere. We squash-merge, so the PR title becomes the commit message. |
snowyukitty
changed the title
* fix: clean up JSStreamer player connections * test(js-streamer): focus shutdown coverage on cleanup --------- Co-authored-by: snowyukitty <[email protected]>
Relevant components
JavaScript streamer (
Extras/JSStreamer) and frontend integration tests.Problem statement
Fixes #944.
Players are assigned as properties on
playerMap, so theMap.values()loop instopStreaming()never sees them. Stopping leaves their peer connections andstats intervals alive. Individual player disconnection clears its interval but
does not close its local peer connection.
Solution
Use the Map API consistently for player insertion, lookup and removal. Both
shutdown and player disconnection release the stats interval and peer connection
through the same cleanup method. Repeated cleanup is safe, while the existing
disconnect notifications, transport close code/reason and caller-owned media
tracks are preserved.
Documentation
Added a JSStreamer patch changeset.
Test Plan and Compatibility
the TypeScript frontend implementation, and JSStreamer with Node v22.14.0.
JSStreamer, media streams and data channels: 14 passed, 3 existing skips.
The new tests cover two-player shutdown, individual disconnection, repeated and
unknown disconnection, idle/repeated shutdown, timer cleanup, transport close,
and media-track ownership. The shutdown tests contain no protocol/settings
delivery assertions.
peer-cleanup assertions; the other 12 tests pass, with the same 3 skips.
it was not rerun for this test-only follow-up.
Browser tests ran in an isolated Linux container using a local TURN fixture.
Firefox's existing basic-stream test could not gather usable ICE candidates in
the original isolated validation, so Firefox compatibility is not established.
An Unreal Editor smoke test was not run.
The October 21 comparison found JSStreamer's existing unsafe-typing lint errors
(39 on the base, 22 on the candidate), and the same 7 existing diagnostics in an
additional frontend-test type check, with none in the new test. Those lint/type
checks were not rerun for this follow-up; production code is unchanged from the
previous PR head.