Potential fix prejoin concurrency

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: c50b8d9893dfdf36e588c2afcf3433122cd9a6c4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[lukasIO]

Thanks for the PR. unfortunately this is pretty much exactly what we had before and it still didn't release tracks reliably. The idea of not relying on tracks as state was that we would always cleanup (and stop) only the tracks created locally from this effect, by assigning it to the localTracks variable and only acting on it. This seems to be in line with what the react docs suggest for clean effects.

[tbscode]

Ok yeah I see that. The issue being random makes this quite hard to test.

I've updated the fix it does the following things now:

1) Also call videoTrack.stop() in the unmount 2) Move the !isCreatingTrack to the front of the if check 3) Add extra orphanTracks to usePreviewTracks

I think (3) specificly fixes the case that:

• when the component unmount is already triggered
• but the track was not created yet though createLocalTracks
• then .stop() on unmount would be called before the track creation finishes ( i.e.: It wouldn't stop )

Now tracks that are created after unmounting, are cleared though orphanTracks.

In my test it's working now.

[tbscode]

This does indeed also seem to fix the issue also with StrictMode.

Here is the new stackblitz: https://stackblitz.com/edit/vitejs-vite-tmapcu?file=src%2FApp.jsx

I've published and installed my patch from my fork directly

[lukasIO]

I think (3) specificly fixes the case that: when the component unmount is already triggered but the track was not created yet though createLocalTracks then .stop() on unmount would be called before the track creation finishes ( i.e.: It wouldn't stop )

you are right that .stop() in unmount could be called before track creation finishes. But unmount also sets needsCleanup = true. And all tracks are stopped right after track creation if needsCleanup is true.

I appreciate your efforts here, but without really understanding the underlying issue, I'm reluctant to merge this PR.

[tbscode]

you are right that .stop() in unmount could be called before track creation finishes. But unmount also sets needsCleanup=true. And all tracks are stopped right after track creation if needsCleanup is true.

I wonder if this is true. Can it not be that needsCleanup=false in the moment the if check runs but then needsCleanup=true turns true after the check if(needsCleanup) completed and thus the cleanup is never called?

I'm not certain how the lock works exactly but afaik it doesn't prevent that?

But yes it would certainly be good if someone with better knowledge of react async would take a look, we will happily help out as this is urgent to us.

I appreciate your efforts here, but without really understanding the underlying issue, I'm reluctant to merge this PR.

Certainly understood, just wanted to start the effort let me know if I can help getting there somehow.

[tbscode]

Also I wonder If we would be off much worse just not using async in useEffect ?

[tbscode]

Updated main and removed the code you marked, still the stack-blitz is working even with StrictMode so this seems to definitely help. We have for now published a patch that we can use internally ( the commits in the pr do indeed seem fix all our issues ), but maintaining a fork is not a sustainable solution, looking forward to finding a better fix and getting it merged here, any help appreciated.

[lukasIO]

I'm unable to reproduce the issue with v2.2.1. If this is urgent to you, you can help out by taking the code of v2.2.1, adding debug statements to the use effect and its cleanup function and try to understand under which circumstances the localTracks are not being stopped.

[tbscode]

Repoduced the same with StrictMode on v2.2.1. And It's prob fair to assume it reprocuces without Strict mode too ( e.g.: fast navigation ). https://stackblitz.com/edit/vitejs-vite-ifhz43?file=package.json

[lukasIO]

I suspect usePreviewTracks is not where the issue actually lies. Can you still reproduce it with this boiled down version that only uses the usePreviewTracks hook? https://stackblitz.com/edit/vitejs-vite-w4bkhn?file=src%2FApp.tsx,src%2FPreJoin.tsx,src%2Fmain.tsx&terminal=dev

[tbscode]

Ok yes it seems not to reproduce with your stack blitz, even when quickly clicking the toggle multiple times. Could still be an issue with how usePreviewTracks is being used or if the video track is attached somewhere when it's stopped. I'll give it another thought, thanks for the stackblitz.

[lukasIO]

found the underlying issue in the client-sdk-js repo: https://github.com/livekit/client-sdk-js/pull/1131. Will close this PR, but thanks for your contribution!

[tbscode]

Thanks! I'll test this to our staging env coming days and report back if it works for us!