Handle engine teardown for add-to-app and hot restart scenarios - #25
Merged
Conversation
onDetachedFromEngine does not fire on hot restart. The FlutterEngine and its id are unchanged while the Dart isolate is replaced, so every registered NativeCallable silently goes stale with no notification -- and invoking a callable whose isolate is gone is undefined behaviour. The plugin now registers a FlutterEngine.EngineLifecycleListener and clears the registrations in onPreEngineRestart(). onEngineWillDestroy() clears too; it fires just before the plugin registry is destroyed, while the engine is still valid, so it is a slightly earlier point than the detach hook. The listener is removed on detach. Only the callback bridges are cleared here, not the whole engine: after a hot restart the new isolate's init() finds the native engine still initialized and deinits it itself. Compiled under -Xlint:all against stubs mirroring the real embedding API, and the JNI symbol name diffed against javac -h output. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two related fixes for apps whose process outlives a FlutterEngine -- routine for a foreground-service audio app such as audio_service. 1. The engine-scoped callback clear no longer blocks the platform thread. clearDartCallbackRegistrationsForEngine() took init_deinit_mutex and loadMutex, and runs on the Android platform thread from onDetachedFromEngine(). init_deinit_mutex is held for the whole of dispose(), which joins the lifecycle scheduler and can inherit a stalled device stop, so detaching while a device operation was in flight could park the UI thread and ANR. Nulling the three global bridges is what actually stops native code invoking a dead NativeCallable, and needs only dart_callback_invocation_mutex, which is never held across a device operation. The per-BufferStream callbacks live inside Player and still need init_deinit_mutex; that part now tries the lock and, on failure, hands the work to a worker that can afford to wait. A failed try_lock must NOT be read as "a dispose is in flight and will do this for me": init_deinit_mutex is also held by loadFile(), loadMem(), initEngine(), changeDevice(), the device start/stop calls and isInited(), none of which clear callbacks. A file load holds it across disk I/O and decode, so a detach landing during one would otherwise silently leave every BufferStream callable pointing at a dying isolate. Doing the Player part outside dart_callback_invocation_mutex also removes a lock-order inversion against disposeSound(), which holds sounds_mutex across soloud.stop() and reaches voiceEndedCallback(). 2. Destroying an engine now tears the native engine down. Previously detach only cleared the bridges, leaving an initialized engine, a live output device and a running scheduler with no Dart able to drive them. requestEngineTeardownForEngine() drops the bridges synchronously and hands the blocking teardown to a detached worker. Both workers capture an initialization generation, bumped by prepareEngineInit(), and abort if a replacement engine initialized while they were waiting -- so a late teardown can never dispose a live engine, and a late clear can never erase callbacks a new engine just registered. Verified: a harness modelling a loadFile() holding the mutex across its I/O shows the old logic performing 0 clears while the new logic performs the clear once the lock frees, with the caller returning in ~280us. A 200-trial race harness confirms a stale teardown never disposes a replacement engine, and a 100-trial one the same for the deferred clear. Both clean under ThreadSanitizer. JNI symbol names diffed against javac -h output. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ycle Android: never block the platform thread, and tear down on engine detach
Brings in the Android engine-lifecycle work from PRs #22 and #23, which were never merged to main: hot restart now clears stale Dart callback registrations, the engine-scoped callback clear no longer blocks the platform thread, and destroying a FlutterEngine tears the native engine down instead of leaving a live device with no Dart to drive it. Conflict resolution (both in files main improved after the branch forked): - FlutterSoloudPlugin.java: keep main's retryable ensureNativeLibraryLoaded() and its UnsatisfiedLinkError try/catch rather than the branch's older one-shot loader, and extend the same guard to the new nativeRequestEngineTeardownForEngine() call. - CHANGELOG.md: both sides appended to 4.0.13; all entries kept. Also tightens requestEngineTeardownForEngine() to require strict callback ownership. Accepting an unowned registration was unsafe: it is also the state of a replacement engine that has initialized but not yet registered its callbacks, and engineInitGeneration cannot detect that case because the replacement's prepareEngineInit() runs before the detaching engine captures the generation. A detach could therefore dispose a live engine. Verified: FlutterSoloudPlugin.java compiles clean under -Xlint:all against embedding stubs, both JNI symbol names match javac -h output, bindings.cpp passes g++ -fsyntax-only, flutter analyze reports only the 6 pre-existing info lints in example/tests, and flutter test passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PWTMdp5kHauqc4Jjwp7b5t
Callback ownership is the wrong signal for deciding whether a detaching FlutterEngine may dispose the native engine, and it fails in both directions. dartCallbackOwnerEngineId is unset for the whole of an initialization: Dart registers callbacks only after initEngine() returns, and initEngine() opens the audio device, which takes seconds on Android. An engine destroyed in that window could not tear down the Player, scheduler and device it had just built -- exactly the state this teardown path exists to prevent. It is also still set to the *previous* engine once a replacement has called prepareEngineInit(). A detaching engine was therefore accepted, captured the replacement's already-bumped generation, and went on to dispose a live engine -- or raised engine_shutdown_requested after the replacement lowered it, so the replacement's initEngine() refused to initialize. The generation counter cannot separate these: it is bumped when an initialization starts, which is before the teardown reads it. prepareEngineInit() now takes the owning engine id and claims the native engine under engine_lifecycle_mutex, alongside the generation and the shutdown flag. A teardown is accepted only while that claim still names the detaching engine, and its worker re-checks the whole claim before disposing. Verifying the claim and raising the shutdown flag in one critical section is what stops a detach cancelling a replacement's initialization. dartCallbackOwnerEngineId now decides only whose callable pointers may be cleared, which is all it was ever able to answer. Also fixes the Java teardown flag, which was set before the library load and the JNI call, making a failure of either terminal: onDetachedFromEngine() found teardownRequested already set and returned, so the retry the lazy loader is documented to allow could never happen. Verified with a harness that #includes the claim helpers verbatim from bindings.cpp so it cannot drift, over five scenarios plus a 400-trial race: gate gap overlap ordinary deinit race callback-owner-or-unowned ok BROKEN ok BROKEN 95/400 strict callback owner BROKEN BROKEN ok ok 400/400 lifecycle ownership ok ok ok ok 400/400 Race column under ThreadSanitizer, which surfaces the interleaving far more often than default timing (1/400). No TSan race reports. bindings.cpp passes g++ -Wall -Wextra, the plugin compiles clean under -Xlint:all against embedding stubs, flutter analyze reports only the 6 pre-existing info lints, and flutter test passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PWTMdp5kHauqc4Jjwp7b5t
Since the teardown hook replaced the plain callback clear on the Java detach path, requestEngineTeardownForEngine() is the only thing that retires Dart callables when a FlutterEngine is destroyed. Gating that clear on the lifecycle claim therefore left a hole: an engine can own the callables without owning the claim. initEngine() carries no ownership token, so the order in which two engines call prepareEngineInit() does not determine the order their initialization workers acquire init_deinit_mutex. Engine A can claim, engine B can claim after it, and A's worker can still be the one that initializes the Player. A then registers its callables -- registration only happens after initEngine() returns -- leaving callback owner A with lifecycle owner B. Destroying A was correctly refused the engine teardown, but returned before clearing anything, so native code kept invoking callables belonging to a dead isolate. That is the undefined behaviour this path exists to prevent. The clear now runs before the lifecycle gate and stays scoped by callback ownership, which is the question it was always able to answer. It cannot touch another engine's callables: clearDartCallbackRegistrationsForEngine() returns early unless the caller owns the registration. Harness scenario 6 drives that interleaving directly and asserts the callables are retired on a refused teardown: it fails with the clear after the gate and passes with it before. Scenarios 1-5 and the 400-trial race are unchanged, clean under ThreadSanitizer. This does not close the underlying ownership gap -- initEngine(), setDartEventCallback(), requestEngineShutdown() and dispose() still take no ownership token, so a stale initialization can still run under a newer engine's claim, and a stale deinit can still dispose a live engine. Fixing that needs the initialization lease threaded through those calls, which changes exported signatures the prebuilt web wasm depends on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PWTMdp5kHauqc4Jjwp7b5t
prepareEngineInit() claimed the native engine, but nothing downstream carried that claim. initEngine() took only the audio configuration and checked a process-global shutdown flag, so calling prepareEngineInit() first did not determine which initialization worker reached init_deinit_mutex first. Three consequences, all reachable with two FlutterEngines: - A superseded initialization could build over the engine that replaced it. It then registered its callables, leaving callback owner A with lifecycle owner B -- and A's later teardown was refused, so its callables outlived its isolate. - setDartEventCallback() validated nothing, so an engine returning from Dart after being superseded overwrote the live engine's callables, silently, with the replacement still believing its registration was current. - requestEngineShutdown() and dispose() were unscoped, so a queued deinit worker could dispose a live engine. disposeLocked() then released whatever claim was current, leaving the replacement initialized but unowned -- and an unowned engine can never be torn down when its FlutterEngine is destroyed. prepareEngineInit() now returns a lease. initEngineOwned(), setDartEventCallback(), requestEngineShutdown() and disposeForEngine() take (engineId, lease) and refuse to act once it is no longer current. initEngineOwned() revalidates after Player::init() returns, since the device open blocks for seconds and ownership can change underneath it; a stale initialization disposes the Player it just built before releasing the mutex, rather than leaving an orphaned device and scheduler running. disposeLocked() takes the claim it is entitled to retire and leaves a claim that has moved on alone. initEngine() and dispose() keep their signatures and become ownership-unaware wrappers. The web build binds those two exported symbols directly from the committed prebuilt wasm, which cannot be regenerated here, and web has no FlutterEngine lifecycle for a lease to describe. Dart holds the claim in the FFI binding rather than threading it through SoLoud: the binding is already one-per-isolate and an isolate belongs to exactly one FlutterEngine, so those fields are that engine's claim. Adds four test-only native barriers (before the init lock, after init succeeded, before callback registration, before the dispose lock) following the existing debugTriggerAudioInterruption precedent. The interleavings that matter are inside the initialization worker while it holds init_deinit_mutex, which no Dart-side sequencing can reach. Harness now covers 10 scenarios including a stale init refused by its lease, a claim lost during the device open (orphan disposed), a stale deinit refused with the live claim intact, and a superseded engine's late registration refused. All pass, clean under ThreadSanitizer. Scenario 6 was rewritten: the lease makes its original premise unreachable, so it now builds the callback/lifecycle owner split the way that is still reachable. flutter analyze unchanged at the 6 pre-existing info lints; flutter test passes; bindings.cpp clean under -Wall -Wextra; plugin clean under -Xlint:all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PWTMdp5kHauqc4Jjwp7b5t
Two lifecycle entry points validated the lease through a helper that took and released engine_lifecycle_mutex, then acted after releasing it. That is the same check-then-act window already closed in tryBeginEngineTeardown(), left open in the two places that were converted to the lease later. requestEngineShutdown(): a replacement engine's prepareEngineInit() could claim and lower the shutdown flag between the lease check and the store, after which the store raised it again and the replacement's initEngine() returned backendNotInited. The replacement-cancellation race, relocated into the ordinary Dart shutdown path. setDartEventCallback(): a replacement could claim between the lease check and the pointer publication, so the superseded engine's callables overwrote the live engine's anyway -- exactly what validating the lease was meant to stop. Both now validate and act inside one engine_lifecycle_mutex critical section. That means engine_lifecycle_mutex is held while acquiring dart_callback_invocation_mutex, so it is no longer a leaf lock; nothing takes those two in the opposite order, and the ordering rule is now stated where the mutex is declared. setDartEventCallback() also returned void, so a refusal was invisible to Dart. The isolate created three NativeCallables, called a void function, and carried on to initialize the loader and publish _nativeCallbacksInitialized = true -- believing it was fully initialized with no callbacks registered against a native engine belonging to another FlutterEngine. It now returns whether the registration was accepted; on refusal Dart closes the callables it just created and throws SoLoudInitializationSupersededException, which the existing handler in init() turns into a failed initialization. The teardown that handler attempts is scoped to the same stale lease and is correctly refused, so it cannot dispose the replacement's engine either. Harness scenarios 11 and 12 drive both windows from a second thread, since the atomic form shuts the replacement out by holding the mutex and an inline interleave would only self-deadlock. Built both ways: the non-atomic form fails both (B's initialization cancelled; callables and lifecycle owner disagree), the shipping form passes. Scenario 12 asserts the invariant that holds whichever engine wins the race -- the live callables belong to the lifecycle owner -- rather than a fixed winner. All 12 pass, clean under ThreadSanitizer. flutter analyze unchanged at the 6 pre-existing info lints; flutter test passes; bindings.cpp clean under -Wall -Wextra. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PWTMdp5kHauqc4Jjwp7b5t
Reverts 69cdd85 and 69e2dcf. Both are correct, but every scenario they address requires two concurrent FlutterEngines, and this plugin's consumer runs exactly one: AudioServiceActivity provides a process-global cached engine via AudioServicePlugin.getFlutterEngine() with shouldDestroyEngineWithHost() = false, so the engine deliberately outlives the Activity and is never replaced by a second one. What that leaves in place is what a single-engine foreground-service app actually needs: tearing the native engine down when its FlutterEngine is destroyed rather than leaving a live output device and lifecycle scheduler with no Dart to drive them, and gating that on the lifecycle claim rather than callback ownership -- which matters single-engine too, since the window where initEngine() has opened the audio device but Dart has not yet registered callbacks is seconds on Android/AAudio. Preserved on claude/engine-lifecycle-lease, including the lease plumbing, the test-only lifecycle barriers, SoLoudInitializationSupersededException and the 12-scenario harness. Restore it if a second engine is ever introduced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PWTMdp5kHauqc4Jjwp7b5t
Fixes RELIEFMIX-3MP, which is two distinct failures reaching the same symptom: the app believes it is playing and produces silence. Android -- SoLoudUnknownErrorException from startAudioDevice(). Every miniaudio failure is flattened to UNKNOWN_ERROR by soloud_miniaudio_resume(), so "an unknown error on the C++ side" means precisely that ma_device_start() failed and the reason was discarded. There was no retry and no recovery anywhere on that path, so one failure left the device stopped indefinitely. The stream is kept open across an idle stop, so a device stopped for a while can be holding a stream the OS has since invalidated. AAudio only reports a disconnect through the error callback of a *running* stream, so a stream torn down while stopped -- a route change, or the framework reclaiming resources from a backgrounded app -- is never rerouted by miniaudio, and the staleness surfaces only here. That matches the field reports: both Android users are on Android 16, and one had been backgrounded 16 hours with repeated low-memory warnings before pressing play. It also explains the first-seen date: 4.0.13 is the release that started stopping the Android device when idle, so before it a stale restart was not reachable. performAudioDeviceStart() now rebuilds the device and retries once. Callers hold mDeviceLifecycleOperationMutex so this cannot interleave with another device operation, and voices, sources and filters live in SoLoud rather than in the device, so playback resumes where it left off. One retry only: a freshly built device that will not start is not failing from staleness. iOS -- the same issue collected events where startAudioDevice() returned success and the device stayed stopped. That is Player::startAudioDevice() returning noError under mInterruptionActive without touching the device. The flag is set from miniaudio's AVAudioSession observer and cleared only by init()/deinit(), and iOS does not reliably deliver AVAudioSessionInterruptionTypeEnded -- notably when the interruption ends while the app is backgrounded. One missed notification latched it true and made every later start a silent no-op for the life of the engine. The event timing agrees: five failures within twenty seconds, identical each time, which is a latched state rather than a transient race. An explicit start is an authoritative request from the app, so it now clears the latch and performs the start. If an interruption really is still in force the OS refuses to activate the session and the start fails, surfacing a real error instead of a false success; a genuine new interruption sets the flag again through the normal callback. Adds a regression test for the iOS half: interrupt, drop the matching "ended" notification, and assert an explicit start still reaches started. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PWTMdp5kHauqc4Jjwp7b5t
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.
Description
This PR fixes a critical issue where the native audio engine would remain initialized and keep an output device running after its owning FlutterEngine is destroyed in add-to-app scenarios or during hot restart. This could lead to undefined behavior when native code attempts to invoke callbacks into a dead isolate.
Key Changes
Engine Lifecycle Management: Added
requestEngineTeardownForEngine()to properly tear down the native engine when a FlutterEngine is destroyed while the process continues running (add-to-app, foreground services, etc.).Generation-Based Synchronization: Introduced
engineInitGenerationatomic counter to prevent race conditions where a queued teardown from a detached engine could incorrectly dispose a replacement engine that initialized while the worker was waiting for locks.Non-Blocking Teardown: The blocking teardown operations (device stop, scheduler join) are delegated to detached worker threads to avoid ANR (Application Not Responding) on the Android platform thread.
Hot Restart Support: Added
onPreEngineRestart()lifecycle listener to clear Dart callback bridges during hot restart, allowing the new isolate to properly reinitialize.Refactored Callback Clearing: Split callback clearing into:
clearDartCallbackPointersLocked(): Makes global Dart bridges inert (fast, no blocking)clearPlayerDartCallbackRegistrationsLocked(): Clears per-BufferStream callbacksqueuePlayerDartCallbackClear(): Non-blocking variant for platform threadAndroid Plugin Updates: Enhanced
FlutterSoloudPluginto:EngineLifecycleListeneron attachmentonEngineWillDestroy()andonDetachedFromEngine()Technical Details
clearDartCallbackRegistrationsForEngine()to avoid blocking the platform threaddispose()behavior is preservedType of Change
Testing
The fix addresses a race condition that manifests in add-to-app and hot restart scenarios. Existing unit tests continue to pass. Manual testing should verify:
https://claude.ai/code/session_01PWTMdp5kHauqc4Jjwp7b5t