Skip to content

Safe join on Drop#156

Open
Roms1383 wants to merge 6 commits intotesselode:mainfrom
Roms1383:fix/safe-join
Open

Safe join on Drop#156
Roms1383 wants to merge 6 commits intotesselode:mainfrom
Roms1383:fix/safe-join

Conversation

@Roms1383
Copy link
Contributor

@Roms1383 Roms1383 commented Mar 7, 2026

This PR keeps spawned thread handle in StreamingSoundHandle to join it on Drop

StreamingSoundHandle size increases from 192 bytes to 216 bytes.
If this is of concern, I could gate it behind a feature gate.

While it's probably unnecessary in most contexts, I depend on kira in audioware which runs in a video game plugin context.

In this particular context, the game can reclaim plugin's resource anytime (e.g. when player kill game in Windows taskbar), which, at times, causes race condition leading to CTD like this:
Screenshot_2026-02-06_135653

This PR also bumps cpal to latest 0.17.3 as it's the continuity of the fix made there in PR 1098 for the very same reason, and available since 0.17.2.

@Roms1383
Copy link
Contributor Author

Roms1383 commented Mar 8, 2026

Ok I tested it with the plugin in-game and it works great. Since the tests are passing, I think it's ready for review 😃

@Roms1383 Roms1383 marked this pull request as ready for review March 8, 2026 09:53
@Roms1383 Roms1383 marked this pull request as draft March 8, 2026 10:13
@Roms1383
Copy link
Contributor Author

Roms1383 commented Mar 8, 2026

This being said I was reading other part of the code and noticed there's already a Drop implementation for StreamingSound. I'm gonna try simply updating cpal first as it seems my additional Drop implementation is probably unnecessary. I wished I noticed it before.

@Roms1383
Copy link
Contributor Author

Sorry for the confusion, we also had a SDK update landing in the meantime which required changes on our side.
I'm gonna take some more time to do additional testing and possibly re-work this PR.
Thanks for your patience and understanding.

@Roms1383
Copy link
Contributor Author

Roms1383 commented Mar 13, 2026

Ok, so the reasoning behind this PR is:

  • no panic inside Drop for SendOnDrop<T>
  • uses thread park with timeout instead of sleep to allow thread to wake spuriously
  • no thread handle left unjoined on Drop
    • for StreamManagerController
      uses a Option<JoinHandle<()>> since there's a single instance of it (24 bytes)
    • for StreamingSound
      uses an AtomicPtr<JoinHandle<()>> (8 + 1 bytes) since there can be multiple instances, it keeps Shared size at 24 bytes (versus 16 previously)
  • bump cpal to benefit from the same guarantee, as explained above

@Roms1383 Roms1383 marked this pull request as ready for review March 13, 2026 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant