Fix static sound disconnect edge case #16846
Conversation
In rare cases when lots of static sounds are playing, it's possible for static sound instances to have their `_onEnded` handler called by the underlying AudioBufferSourceNode *after* the sound has been disposed. This causes the static sound instance `_disconnect` call to occur twice, which throws an error on the 2nd call since it is already disconnected. This change adds an `_isConnected` flag to static sound instances which skips the 2nd call to `_disconnect` if it is already disconnected.
There was a problem hiding this comment.
Pull Request Overview
This PR prevents a static sound instance from attempting to disconnect twice by introducing an _isConnected flag and guarding the _disconnect call.
- Added an
_isConnectedflag to_WebAudioStaticSoundInstance - Set/reset the flag when connecting/disconnecting
- Updated the
_onEndedhandler to skip a second disconnect
Comments suppressed due to low confidence (4)
packages/dev/core/src/AudioV2/webAudio/webAudioStaticSound.ts:310
- [nitpick] The flag name
_isConnectedis ambiguous—consider renaming it to something like_hasActiveConnectionor_isConnectedToNodeto make its purpose clearer.
private _isConnected: boolean = false;
packages/dev/core/src/AudioV2/webAudio/webAudioStaticSound.ts:310
- Add a comment explaining why
_isConnectedis needed to guard against double-disconnect errors when the AudioBufferSourceNode triggers itsonendedafter disposal.
private _isConnected: boolean = false;
packages/dev/core/src/AudioV2/webAudio/webAudioStaticSound.ts:521
- Add a unit test that simulates the
onendedevent firing after dispose to verify that_disconnectis called only once and no error is thrown.
if (this._isConnected && !this._disconnect(this._sound)) {
packages/dev/core/src/AudioV2/webAudio/webAudioStaticSound.ts:488
- The flag is only set when connecting to a
_WebAudioStaticSoundnode; if the instance connects to other node types,_isConnectedremains false and may skip needed disconnects. Consider setting the flag for all connect calls or scoping the logic more precisely.
this._isConnected = true;
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
|
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16846/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/16846/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/16846/merge#BCU1XR#0 |
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
In rare cases when lots of static sounds are playing, it's possible for static sound instances to have their `_onEnded` handler called by the underlying AudioBufferSourceNode *after* the sound has been disposed. This causes the static sound instance `_disconnect` call to occur twice, which throws an error on the 2nd call since it is already disconnected. This change adds an `_isConnected` flag to static sound instances which skips the 2nd call to `_disconnect` if it is already disconnected.
In rare cases when lots of static sounds are playing, it's possible for static sound instances to have their
_onEndedhandler called by the underlying AudioBufferSourceNode after the sound has been disposed. This causes the static sound instance_disconnectcall to occur twice, which throws an error on the 2nd call since it is already disconnected.This change adds an
_isConnectedflag to static sound instances which skips the 2nd call to_disconnectif it is already disconnected.