fix: resolve Android stop() deadlock (self-join + EOS-never-queued race) - #508
Open
iawaisrana wants to merge 3 commits into
Open
fix: resolve Android stop() deadlock (self-join + EOS-never-queued race)#508iawaisrana wants to merge 3 commits into
iawaisrana wants to merge 3 commits into
Conversation
stopEncoder() ran on its own handlerThread and called handlerThread.join() on itself, deadlocking forever and preventing the completion callback from firing. RecorderController.stop() would then hang indefinitely on Android whenever the AAC/CommonEncoder path was used. Removes the self-join, invokes the completion callback before quitting the handler thread instead of after, posts MediaCodec callbacks and the method channel result onto explicit handlers for thread-safety, and adds @volatile to fields read across threads. Cherry-picked from upstream PR SimformSolutionsPvtLtd#486.
signalToStop() relies on a future onInputBufferAvailable callback to queue EOS when no input buffer is available at the moment stop() is called. But recording has already stopped feeding new audio by then, so that callback may never arrive, leaving stopEncoder() (and the completion callback / Dart-side stop() future) hanging indefinitely. Reproduced consistently on a second back-to-back recording. Add a 500ms grace-period fallback: if EOS hasn't been queued naturally by then, force stopEncoder() directly. It's idempotent and always invokes the completion callback, so the Dart Future can never hang.
Different devices/Codec2 implementations may need more or less time before the natural EOS path can be considered stalled. Hardcoding 500ms risked either forcing a stop too early on slower devices (truncating the recording's tail) or not being adjustable for apps that hit this more severely. Adds AndroidEncoderSettings.stopTimeoutMs (default 500, matching the previous hardcoded value), threaded through to RecorderSettings on both the Dart and native side, down to CommonEncoder.signalToStop()'s postDelayed fallback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes the Android
stop()hang reported in #470, combining two changes:CommonEncoder.stopEncoder()(it calledhandlerThread.join()from a callback running on that same thread).signalToStop()runs while no input buffer is currently available, it waits for a futureonInputBufferAvailablecallback to queue EOS. SinceaudioRecord.stop()has already run by that point, that callback can simply never arrive on some devices, hanging forever even with fix: Android recorderController.stop() never completes #486 applied. This PR adds a grace-period fallback: if EOS isn't queued naturally in time,stopEncoder()is forced directly (it's already idempotent and always invokes the completion callback).Why configurable
How long the natural path takes likely varies by device/OEM Codec2 implementation, so the grace period is exposed as
AndroidEncoderSettings.stopTimeoutMs(default 500ms, matching the previous hardcoded value) rather than hardcoded.Testing
Reproduced and verified via logcat on a physical device: without a fix,
stop()on a second/third recording in a session would hang indefinitely with no furtherMediaCodec/CommonEncoderlog output at all. With this fix, the fallback fires and logs a warning, then completes the stop within the grace period.I didn't add automated tests — the repo has no existing test infrastructure for either the native Android side or the Dart side, and the actual bug is a device/OEM-dependent async timing race that isn't practical to reproduce deterministically in a unit test. Happy to add coverage if there's a preferred approach for this.