Allow aborting loading audio #4017
Conversation
This change makes loadAudio optionally take an AbortSignal which can be used to abort loading. We use this to prevent a race condition where the wrong audio might be loading into wavesurfer.
WalkthroughThis pull request enhances the WaveSurfer class by introducing an optional AbortSignal parameter into the asynchronous audio loading methods. The loadAudio method now includes multiple checkpoints for aborting operations using signal?.throwIfAborted(), while the load and loadBlob methods are updated to pass the signal parameter to loadAudio. The changes improve error handling and allow audio loading processes to be canceled when needed. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant WaveSurfer
participant AbortSignal
Caller->>WaveSurfer: load(url, ..., signal)
WaveSurfer->>WaveSurfer: Begin fetching audio blob
alt Check after fetch
AbortSignal-->>WaveSurfer: throwIfAborted()
WaveSurfer->>Caller: Abort error thrown
else
WaveSurfer->>WaveSurfer: Set duration & decode audio data
alt Check after decoding
AbortSignal-->>WaveSurfer: throwIfAborted()
WaveSurfer->>Caller: Abort error thrown
else
WaveSurfer->>Caller: Return loaded audio
end
end
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
src/wavesurfer.tsOops! Something went wrong! :( ESLint: 9.17.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](/~https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/wavesurfer.ts (1)
431-434: Consider using the provided signal for fetch operations.The code creates a new
AbortControllerwhen none is provided, but it could potentially reuse the provided signal for fetch operations to ensure consistent abort behavior.Consider this implementation:
if (window.AbortController && !fetchParams.signal) { - this.abortController = new AbortController() - fetchParams.signal = this.abortController?.signal + this.abortController = signal ? undefined : new AbortController() + fetchParams.signal = signal || this.abortController?.signal }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/wavesurfer.ts(5 hunks)
🔇 Additional comments (3)
src/wavesurfer.ts (3)
421-421: LGTM! The signal parameter is properly added to method signatures.The optional
AbortSignalparameter is consistently added to all three methods:loadAudio,load, andloadBlob.Also applies to: 487-488, 497-498
489-491: LGTM! Error handling is properly maintained.The signal parameter is correctly forwarded to
loadAudiowhile maintaining the existing error handling pattern.Also applies to: 499-501
437-437: Verify abort signal checks after async operations.The abort signal checks are strategically placed after each async operation:
- After fetching the blob
- After getting the audio duration
- Before and after decoding the audio data
This ensures that the loading process can be aborted at any point during these potentially time-consuming operations.
Let's verify the abort signal usage with a test script:
Also applies to: 459-459, 474-476
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for consistent abort signal usage patterns in the codebase # Look for other async operations that might need abort signal checks rg -A 5 "await.*\(" --type ts # Look for existing abort signal usage patterns rg "signal\?\.throwIfAborted\(\)" --type tsLength of output: 3886
Abort Signal Usage Verification in Audio Loading Operations
Based on the grep output:
• The abort signal check ("signal?.throwIfAborted()") is confirmed immediately after fetching the blob.
• It is also used after the blob’s arrayBuffer conversion and after decoding the audio data.
• While the review comment noted a signal check after retrieving the audio duration, our current search output does not conclusively show a check immediately following that duration promise. However, all other abort checks for the async operations in question are correctly placed.Overall, the abort signal checks are consistently applied after the major async operations in wavesurfer.ts. You may want to confirm that any async operation that could be long-running—such as fetching or decoding—is safeguarded by an abort check if intended. In this case, the placement matches the intended strategy described in the review comment.
|
You can already pass an abort signal in the fetchParams option. See https://wavesurfer.xyz/docs/types/wavesurfer.WaveSurferOptions Or is it not sufficient for your use case? |
|
We're using a singleton wavesurfer instance and loading different audio into it. When the second call to |
|
Ah, so you aren't even interested in aborting the actual fetch or decoding process, only in interrupting the whole thing in between those async processes as if they threw? |
| @@ -434,6 +434,8 @@ class WaveSurfer extends Player<WaveSurferEvents> { | |||
| } | |||
| const onProgress = (percentage: number) => this.emit('loading', percentage) | |||
| blob = await Fetcher.fetchBlob(url, onProgress, fetchParams) | |||
There was a problem hiding this comment.
Since there's a bit of an overlap with the existing fetchParams.signal, I'm thinking maybe we can use the new one for this as well?
Something like:
| blob = await Fetcher.fetchBlob(url, onProgress, fetchParams) | |
| blob = await Fetcher.fetchBlob(url, onProgress, { | |
| ...fetchParams, | |
| signal: signal ?? fetchParams?.signal | |
| }) |
Then if you abort it, it will also abort the fetching.
Short description
This change makes loadAudio optionally take an AbortSignal which can be used to abort loading.
We use this to prevent a race condition where the wrong audio might be loading into wavesurfer.
Implementation details
Passing through an AbortSignal. After any async call it needs to be checked with
throwIfAborted. Not my favorite, but is effective.How to test it
Easiest is to add some delays in
loadAudioand then try aborting.Add manual delays throughout loadAudio with:
Then call loadAudio passing in a signal and aborting it.
Screenshots
Checklist
Summary by CodeRabbit