fix: reply exactly once, on the platform thread, from async channel cases - #76
Open
vahidlazio wants to merge 1 commit into
Open
fix: reply exactly once, on the platform thread, from async channel cases#76vahidlazio wants to merge 1 commit into
vahidlazio wants to merge 1 commit into
Conversation
…ases
Three Android cases replied from inside coroutineScope.launch on
Dispatchers.IO with no error handling: fetchAndActivate,
activateAndFetchAsync and readAllFlags. Two defects followed.
If the suspending body threw, result.success was never reached and NO
reply was sent, so the Dart future never completed and the caller hung
forever. Flutter's onMethodCall RuntimeException guard cannot help here
because the throw happens later, on an IO thread. readAllFlags is the
most reachable: a corrupt or partially written flag cache makes
Json.decodeFromString throw.
Flutter also requires channel replies on the platform (main) thread, and
a second reply throws. Neither guarantee held at these call sites, so
both are now enforced by a MainThreadResult wrapper that marshals to the
main looper and drops any reply after the first.
iOS already replied on every path but did so from inside Task {}, which
resumes on an arbitrary executor, so the same wrapper is applied there.
iOS now also surfaces fetch/activate failures as a FlutterError instead
of logging and reporting success, matching Android.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
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.
The defect
Three Android method-channel cases reply from inside
coroutineScope.launch {}onDispatchers.IO(ConfidenceFlutterSdkPlugin.kt, scope declared ~line 31) with no error handling:fetchAndActivate,activateAndFetchAsyncandreadAllFlags. Two problems follow.1. A throwing body sends no reply at all. If the suspending call throws,
result.success(...)is never reached, so the Dart future never completes and the caller hangs forever. Flutter'sonMethodCallRuntimeExceptionguard cannot help, because the throw happens later on an IO thread.Concretely,
readAllFlagsis the most reachable: a corrupt or partially writtenconfidence_flags_cache.jsonmakesJson.decodeFromStringthrow, andawait readAllFlags()in Dart then never returns.2. Replies are made off the platform thread. Flutter requires
MethodChannel.Resultreplies on the main thread; these came fromDispatchers.IO.iOS has the same shape inside
Task {}. It already replied on every path, butTask {}resumes on an arbitrary executor, so problem 2 applied there too.The fix
A small
MainThreadResultwrapper on each platform that marshals the reply to the main thread and drops any reply after the first — a second reply throws on both platforms, so exactly-once has to be enforced, not assumed. Each async body is then wrapped so every path replies: success, or an error carrying the failure.Behaviour change worth calling out
iOS previously caught fetch/activate failures, logged them, and replied success. It now replies with a
FlutterError, matching Android's new behaviour. An app that cannot fetch flags should not be told it succeeded — but this does meanawait fetchAndActivate()can now throw aPlatformExceptionon iOS where it previously resolved silently.Testing
The native change cannot be exercised from Dart, since the Dart tests mock the platform side entirely. Two things are worth knowing about coverage here:
ConfidenceFlutterSdkPluginTest.kt) is not run by CI — theandroid-testjob runsflutter driveintegration tests on an emulator, not./gradlew testDebugUnitTest. That test also still asserts against agetPlatformVersioncase the plugin no longer implements, so it would fail if it were run. Left alone here as out of scope..catchErrorintofetchAndActivatefails withExpected: throws <PlatformException> ... Actual: <Future<void>>.flutter analyzeclean (one pre-existingexample/.envasset warning, created by CI) andflutter test5/5 passing.Scope
Pre-existing on
mainand independent of #75, which fixes the separate missing-reply defect intrackandflush. Thetry!in iOSreadAllFlags/getObjectis a distinct crash risk and is not addressed here.🤖 Generated with Claude Code