edgeTTS fix stop race condition#4
Conversation
WalkthroughRefactors TTS shutdown: stop() now tears down WebSocket handlers and conditionally closes the socket, clears audio state and queues, revokes blob URLs, resets the audio element, and delegates MediaSource finalization to a new event-driven #finalizeStream that uses sourceBuffer.updateend and guarded endOfStream calls instead of polling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant T as TTS Controller
participant WS as WebSocket
participant MS as MediaSource
participant SB as SourceBuffer
participant A as Audio Element
U->>T: stop()
T->>WS: remove handlers (onclose/onerror/onmessage)
alt WS open or connecting
T->>WS: close()
T->>T: null currentSocket
end
T->>T: clear queues/state
T->>A: pause(), remove src, revokeObjectURL (if blob), try load()
alt MediaSource present and open
T->>T: #finalizeStream()
T->>MS: check readyState
opt SB is updating
SB-->>T: updateend (event)
end
T->>MS: endOfStream() (guarded try/catch)
end
note right of T: Cleanup complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wiktionary_pron/scripts/tts.js (1)
262-290: Avoid truncating audio: finalize only after the queue fully drains, and clean up references.Current logic ends the MediaSource after just one
updateend, which can cut off remaining queued chunks. Also, references to#sourceBuffer/#mediaSourcearen’t cleared, risking leaks.Apply this diff to make finalization drain-aware and perform cleanup:
- #finalizeStream() { - // This is the more robust version that prevents Firefox warnings and is generally safer. - if (!this.#mediaSource || this.#mediaSource.readyState !== "open") { - return; - } - - const end = () => { - if (this.#mediaSource.readyState === "open") { - try { - this.#mediaSource.endOfStream(); - } catch (e) { - console.warn( - "Error calling endOfStream, stream likely already closed.", - e, - ); - } - } - }; - - if (this.#sourceBuffer && this.#sourceBuffer.updating) { - const onUpdateEnd = () => { - this.#sourceBuffer.removeEventListener("updateend", onUpdateEnd); - end(); - }; - this.#sourceBuffer.addEventListener("updateend", onUpdateEnd); - } else { - end(); - } - } + #finalizeStream() { + const ms = this.#mediaSource; + if (!ms) return; + + const end = () => { + if (this.#mediaSource && this.#mediaSource.readyState === "open") { + try { + this.#mediaSource.endOfStream(); + } catch (e) { + console.warn("Error calling endOfStream (likely already closed).", e); + } + } + // Remove our queue-drain listener if present + try { this.#sourceBuffer?.removeEventListener("updateend", this.#onQueueDrain); } catch {} + this.#onQueueDrain = null; + // Drop strong refs to allow GC + this.#sourceBuffer = null; + this.#mediaSource = null; + this.#isAppending = false; + }; + + const tryEndNow = () => { + if (!this.#sourceBuffer || (!this.#sourceBuffer.updating && this.#audioQueue.length === 0)) { + end(); + } + }; + + if (this.#sourceBuffer) { + // Keep appending until the queue is empty, then end. + const maybeEnd = () => { + if (this.#audioQueue.length === 0 && !this.#sourceBuffer.updating) { + this.#sourceBuffer.removeEventListener("updateend", maybeEnd); + end(); + } + }; + this.#sourceBuffer.addEventListener("updateend", maybeEnd); + // Kick the queue in case it’s idle. + this.#processAudioQueue(); + tryEndNow(); + } else { + end(); + } + }
🧹 Nitpick comments (3)
wiktionary_pron/scripts/tts.js (3)
296-307: Avoid leaking bound listeners: keep a stable handler reference.
this.#processAudioQueue.bind(this)creates a new function you can’t remove later, retaining the SourceBuffer. Store the bound handler and remove it in#finalizeStream().try { this.#sourceBuffer = this.#mediaSource.addSourceBuffer( this.#mimeType, ); - this.#sourceBuffer.addEventListener( - "updateend", - this.#processAudioQueue.bind(this), - ); + this.#onQueueDrain = this.#processAudioQueue.bind(this); + this.#sourceBuffer.addEventListener("updateend", this.#onQueueDrain); resolve();Add this private field to the class (anywhere among other fields):
#onQueueDrain = null;
223-233: Undefined vars in caching path (audioChunks,cacheKey). Gate or implement before enabling.This will throw if
#enableCacheis ever set to true. Either remove for now or switch to defined state tracked on the instance.Minimal safe guard:
socket.onclose = () => { this.#finalizeStream(); - if (this.#enableCache && audioChunks.length > 0) { - //Use dynamic mimeType for blob creation --- - const audioBlob = new Blob(audioChunks, { type: this.#mimeType }); - console.log( - `Saving to cache (${(audioBlob.size / 1024).toFixed(2)} KB)`, - ); - this.#cache.saveAudio(cacheKey, audioBlob); - } + if (this.#enableCache && this.#receivedChunks && this.#receivedChunks.length > 0) { + const audioBlob = new Blob(this.#receivedChunks, { type: this.#mimeType }); + console.log(`Saving to cache (${(audioBlob.size / 1024).toFixed(2)} KB)`); + this.#cache.saveAudio(this.#lastCacheKey, audioBlob); + } };To complete this approach, add these fields and set them where appropriate:
// Private fields (with others) #receivedChunks = null; #lastCacheKey = null; // In speak(config), before opening the socket: this.#lastCacheKey = JSON.stringify({ text, voice: voice.raw.ShortName, fmt: this.#apiAudioFormat }); this.#receivedChunks = []; // In socket.onmessage, when you compute `audioData`: if (this.#enableCache && audioData.byteLength > 0) { // Store a copy to avoid mutation surprises this.#receivedChunks.push(audioData.slice(0)); }
571-583: GuardEdgeTTS.init()when constructor failed.If
new StreamingTTS()throws,EdgeTTSisundefinedand calling.init()will throw beforeallSettled. Guard and letallSettledreport rejection.- const results = await Promise.allSettled([ - EasySpeech.init({ maxTimeout: 5000, interval: 250 }), - EdgeTTS.init(), - ]); + const results = await Promise.allSettled([ + EasySpeech.init({ maxTimeout: 5000, interval: 250 }), + EdgeTTS ? EdgeTTS.init() : Promise.reject(new Error("EdgeTTS unavailable")), + ]);
Implements suggestions from the AI PR review to make the shutdown procedure more robust. - Nullifies all WebSocket event handlers (onopen, onmessage, etc.) to prevent lingering callbacks. - Defensively checks WebSocket readyState before calling close(). - Fully resets the audio element by calling load() in a try-catch block.
1 participant
Summary by CodeRabbit