Skip to content

fix: reply exactly once, on the platform thread, from async channel cases - #76

Open
vahidlazio wants to merge 1 commit into
mainfrom
vahidt/flutter-async-reply-safety
Open

fix: reply exactly once, on the platform thread, from async channel cases#76
vahidlazio wants to merge 1 commit into
mainfrom
vahidt/flutter-async-reply-safety

Conversation

@vahidlazio

Copy link
Copy Markdown
Collaborator

The defect

Three Android method-channel cases reply from inside coroutineScope.launch {} on Dispatchers.IO (ConfidenceFlutterSdkPlugin.kt, scope declared ~line 31) with no error handling: fetchAndActivate, activateAndFetchAsync and readAllFlags. 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's onMethodCall RuntimeException guard cannot help, because the throw happens later on an IO thread.

Concretely, readAllFlags is the most reachable: a corrupt or partially written confidence_flags_cache.json makes Json.decodeFromString throw, and await readAllFlags() in Dart then never returns.

2. Replies are made off the platform thread. Flutter requires MethodChannel.Result replies on the main thread; these came from Dispatchers.IO.

iOS has the same shape inside Task {}. It already replied on every path, but Task {} resumes on an arbitrary executor, so problem 2 applied there too.

The fix

A small MainThreadResult wrapper 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 mean await fetchAndActivate() can now throw a PlatformException on 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:

  • The Kotlin unit test file (ConfidenceFlutterSdkPluginTest.kt) is not run by CI — the android-test job runs flutter drive integration tests on an emulator, not ./gradlew testDebugUnitTest. That test also still asserts against a getPlatformVersion case the plugin no longer implements, so it would fail if it were run. Left alone here as out of scope.
  • The added Dart tests therefore pin the contract the native fix depends on: that these futures complete with an error rather than swallowing it or hanging. Verified falsifiable — injecting an error-swallowing .catchError into fetchAndActivate fails with Expected: throws <PlatformException> ... Actual: <Future<void>>.

flutter analyze clean (one pre-existing example/.env asset warning, created by CI) and flutter test 5/5 passing.

Scope

Pre-existing on main and independent of #75, which fixes the separate missing-reply defect in track and flush. The try! in iOS readAllFlags/getObject is a distinct crash risk and is not addressed here.

🤖 Generated with Claude Code

…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant