[DELIVERY-125863] Migrate Flutter plugin to AppsFlyer SDK 7 RPC architecture. - #466
[DELIVERY-125863] Migrate Flutter plugin to AppsFlyer SDK 7 RPC architecture.#466af-dudka wants to merge 66 commits into
Conversation
…tecture. Route core APIs through af-api/af-events RPC bridges on Android and iOS, adopt the SDK 7 init + session-ready start model, remove SDK 6 APIs that no longer exist natively, bump native dependencies to 7.0.1, raise iOS minimum to 13.0, and reorganize public documentation.
Replace legacy INSTALL_REFERRER receiver instructions with Install Referrer library guidance; format appsflyer_sdk.dart and test file for CI.
|
Mostly reviewed Android side - looks nice, great job, Andrii! |
…nd fix analyzer lints
| } | ||
| } | ||
|
|
||
| private fun drain() { |
There was a problem hiding this comment.
A single event that permanently fails to send poisons the queue for every future engine.
drain() treats any false from send() as "the sink died" and stops, leaving the failing
event at the head of pendingEvents. If the failure was actually caused by the event itself
rather than a dead engine (e.g. an oversized af-events payload triggering
TransactionTooLargeException from the Binder, or any other exception events.success() throws
that isn't about engine teardown), createEventSink's catch-all reports it as a refusal, sink
is nulled, and the event stays at index 0. On the next attach() — a brand-new, perfectly live
engine — drain() retries that same poisoned event first, fails the same way, nulls the new
sink again, and returns: every other buffered event is now permanently blocked from reaching any
future engine, even though only one event was ever actually bad.
Distinguish "sink is gone" from "this event could not be sent" — e.g. drop-and-log a single
event after a bounded number of consecutive failures, or skip a poison event rather than
treating a false return as terminal for the whole drain.
There was a problem hiding this comment.
Good catch — confirmed. On refusal we now drop that event and detach the sink so the rest of the buffer can replay on the next attach. Added tests for content-based refusal and updated the comment in createEventSink. One note: TransactionTooLargeException doesn't apply here since af-events doesn't go through Binder.
| AppsFlyerEventBus.attach(sink) | ||
| } | ||
|
|
||
| override fun onCancel(arguments: Any?) { |
There was a problem hiding this comment.
The entire "did the sink survive?" signal for the bus is events.success() throwing. That's not
a documented contract of EventChannel.EventSink — depending on the Flutter embedding
version/implementation, sending on a torn-down BinaryMessenger/FlutterJNI may just silently
no-op (log-and-return) rather than throw, especially when the engine is merely detached-but-not-
yet-destroyed vs. fully released. If that happens, send() returns true, drain() pops the
event as delivered, and it's actually lost — silently defeating the whole point of this change
(no events lost after teardown).
Worth verifying empirically against the embedding versions this plugin supports, not just
asserting the throw-on-teardown behavior via a mock in the unit test, since the fix's core
guarantee rests entirely on this assumption holding on-device.
There was a problem hiding this comment.
Good point — confirmed on device. success() often no-ops on a detached embedding instead of throwing, so treating a normal return as delivered could silently drop events while the bus still held a stale sink.
Fixed by returning RETRY_LATER from createEventSink once isEngineDetached is set (before calling success()), and teaching the bus three outcomes: deliver, retry later, or drop a refused head event. Documented in ARCHITECTURE.md. Verified with the delayed-detach repro: the probe now stays buffered (pending=1) instead of being popped as delivered.
| } | ||
|
|
||
| private fun requireApplicationContext(): Context { | ||
| return applicationContext |
There was a problem hiding this comment.
initFromRpc no longer dispatches to a background executor. It used to run the whole init
sequence inside rpcExecutor!!.execute { ... }; now it calls runRpc(result, ...) { ... }, and
runRpc is just a try/catch — no thread hop. Since a plain MethodChannel (no TaskQueue
configured here) delivers onMethodCall on the platform/UI thread, the entire native SDK
bootstrap (setPluginInfo + initialize, which touches SharedPreferences and inspects the
launch intent) now runs synchronously on the main thread at exactly the point — cold start —
where jank/ANRs are most visible. This also diverges from iOS, which dispatches all RPCs
asynchronously.
// Fix: keep init off the platform thread, same as before this refactor
val executor = blockingRpcExecutor ?: run {
result.error(PLUGIN_DETACHED, RPC_EXECUTOR_UNAVAILABLE_MSG, null)
return
}
executor.execute {
try {
executeRpcSync(RPC_METHOD_SET_PLUGIN_INFO, jsonOf(...))
val init = executeRpcSync(RPC_METHOD_INIT, jsonOf("devKey", afDevKey), initContext)
uiThreadHandler.post {
if (init is RpcResponse.Error) deliverRpcResult(init, result, null) else result.success(null)
}
} catch (t: Throwable) {
uiThreadHandler.post { result.error("INIT_ERROR", t.message, null) }
}
}Longer-term alternative worth a follow-up ticket: BinaryMessenger.makeBackgroundTaskQueue()
is the more idiomatic fix — pass the resulting TaskQueue to MethodChannel's 4-arg constructor
and onMethodCall itself runs off the platform thread, so result.success/error() can be called
directly from that worker thread without the manual uiThreadHandler.post hops used throughout
this file today. Not a drop-in swap for this fix, though: the default queue is serial, so
routing the whole channel through it would just move the head-of-line-blocking problem
isBlockingRpc (:309) exists to avoid — a slow blocking RPC would now stall every other RPC on
the channel instead of the main thread. TaskQueueOptions().setIsSerial(false) (concurrent mode)
avoids that, but removes the implicit main-thread confinement that activity, applicationContext,
isEngineDetached, and blockingRpcExecutor all currently rely on with no locking — adopting it
means auditing and synchronizing all of that first. Land the narrow executor fix above now; track
the TaskQueue migration as separate follow-up work rather than folding it into this PR.
References:
There was a problem hiding this comment.
routed initFromRpc back through blockingRpcExecutor with main-thread result delivery so cold-start bootstrap stays off the platform thread.
| * are released in `onDetachedFromEngine`. | ||
| * | ||
| * This does not carry Dart state across the gap. The application's callbacks lived in the | ||
| * destroyed isolate, so it still has to subscribe to the streams and call the `register*Listener` |
There was a problem hiding this comment.
This file's own KDoc says: "the application's callbacks lived in the destroyed isolate, so it
still has to subscribe to the streams and call the register*Listener APIs again after a new
engine attaches." This diff is exactly the migration away from that model — the plugin no longer
exposes any Dart Stream for these events (see lib/src/appsflyer_listener_registry.dart, and
the removed onConversionDataSuccess/onDeepLinkReceived/onSessionReady streams). Leaving
"subscribe to the streams" in a comment authored by this same commit range misdescribes the
current API.
// current (wrong): "...so it still has to subscribe to the streams and call the
// `register*Listener` APIs again after a new engine attaches; ..."
// corrected: "...so it still has to call the `register*Listener` APIs again after
// a new engine attaches; ..."References:
There was a problem hiding this comment.
Fair point - that line predates the callback migration. Updated the KDoc to drop the streams reference; the app only needs to call register*Listener again after a new engine attaches.
| }); | ||
|
|
||
| test('check getAppsFlyerUID call', () async { | ||
| instance.getAppsFlyerUID(); | ||
| test('unexpected null RPC result throws AppsFlyerException', () async { |
There was a problem hiding this comment.
The _invokeRpc<T extends Object> null-safety refactor makes getHostName()/getHostPrefix()
non-nullable, so a native reply of null must throw AppsFlyerException. This throw-on-null path
is tested here for isSessionReady, isStopped, and isPreInstalledApp, but getHostName/
getHostPrefix are still missing from this list — their only coverage is the platform-only
method-not-found test, which exercises the PlatformException (404) path, not a native call that
succeeds while replying with no value (a scenario
internal-docs/features/F-006-custom-host-configuration.md explicitly calls out).
for (final call in <Future<Object?> Function()>[
iosSdk.isSessionReady,
androidSdk.isStopped,
androidSdk.isPreInstalledApp,
androidSdk.getHostName, // missing
androidSdk.getHostPrefix, // missing
]) { ... }References:
There was a problem hiding this comment.
Good catch — added getHostName and getHostPrefix to the null-result throw test so they match the other non-nullable _invokeRpc getters.
|
|
||
| override fun onCancel(arguments: Any?) { | ||
| releaseEventSink() | ||
| } |
There was a problem hiding this comment.
catch (t: Throwable) is overly broad — it also catches Error subtypes (e.g.
OutOfMemoryError, StackOverflowError, AssertionError) and any unrelated bug thrown from
events.success(...), logging them all as a routine "sink refused an event" and feeding them
back into AppsFlyerEventBus as a normal detach signal. That masks real defects as expected
teardown behavior, and — per the drain() poison-event issue above — can wedge delivery for a
live engine on an exception that has nothing to do with the engine being torn down.
Narrow this to the exception types EventChannel.EventSink/BinaryMessenger are actually
expected to throw after cancellation (e.g. IllegalStateException), or at minimum
RuntimeException, and let anything else propagate instead of being silently absorbed.
There was a problem hiding this comment.
Good catch — the catch (Throwable) → DROP path in createEventSink was too broad and could mask real defects as a routine sink refusal.
Replaced it with catch (RuntimeException): log at error and return RETRY_LATER so the head event stays buffered without taking down the platform thread. Error types still propagate. Teardown remains on the explicit isEngineDetached → RETRY_LATER path before success() is called — Flutter's EventSink.success() does not document throws for cancel/detach on current embeddings (inactive sinks and detached JNI usually return silently).
| private var sink: AppsFlyerEventSink? = null | ||
|
|
||
| /** Queues [eventJson], then flushes as much of the buffer as the attached sink accepts. */ | ||
| fun publish(eventJson: String) { |
There was a problem hiding this comment.
When the buffer exceeds MAX_PENDING_EVENTS, the oldest event is silently dropped
(removeFirst()) with no logging or metric. This directly undercuts the stated purpose of this
change ("native events... were lost" → fix delivers them), just for a different trigger: an app
that doesn't resubscribe for a while (backgrounded a long time, crash loop, etc.) will silently
lose early events with no trace in logs. Worth at least a Log.w when the cap is hit so this
failure mode is diagnosable in production instead of looking identical to "everything worked."
There was a problem hiding this comment.
Fair point — the 64-event cap is intentional, but silent eviction made overflow indistinguishable from a healthy replay. Added a Log.w when we drop oldest pending events so that failure mode shows up in production logs.
| * Delivery is FIFO: events are queued first and flushed in publish order, so a replayed event | ||
| * always precedes one published after it. | ||
| * | ||
| * **Threading**: every entry point is synchronized, so publishing from an SDK callback thread is |
There was a problem hiding this comment.
The class doc overclaims what the lock actually guarantees: "every entry point is synchronized,
so publishing from an SDK callback thread is safe." synchronized(lock) on a private Any() is
the right primitive for this plain (non-coroutine) singleton — no argument there. But drain()
calls the external AppsFlyerEventSink.send(...) (which wraps EventChannel.EventSink.success(),
main-thread-only per Flutter's own contract) while the lock is held, inside both publish() and
attach(). That's only safe today because every real caller (rpcEventNotifier's
uiThreadHandler.post, and onListen/onCancel, in AppsflyerSdkPlugin.kt) already funnels
through the Android main thread before reaching the bus — the lock itself does nothing to enforce
that. A future caller invoking publish() directly from a background thread would run
EventSink.success() off the main thread while any other thread blocks on lock for the
duration of that call — a real cross-thread hazard the docstring's "safe" claim currently hides.
Don't fix this by releasing the lock around send() and re-acquiring to commit state: that opens
a worse latent bug than the one it closes — two concurrent drain() calls could both peek the
same head event and each deliver it to a different sink before either removes it, producing
duplicate delivery and breaking the exactly-once-FIFO guarantee this class exists for. Prefer
either asserting/documenting that all entry points must already be on the main thread (matching
actual usage), or reword the docstring so it states the lock protects internal state consistency,
not that send() itself runs on the right thread — that responsibility stays with the caller,
same as it is today.
References:
There was a problem hiding this comment.
Agreed — the lock protects buffer/sink consistency, not main-thread delivery. drain() calls the sink on the caller's thread, and we rely on AppsflyerSdkPlugin to post publish() onto the main looper and to call attach/detach from the platform thread. Reworded the KDoc to state that explicitly. Keeping the lock held across send() so FIFO exactly-once delivery cannot race.
|
|
||
| /// Invokes `FlutterResult` only while this engine instance is still attached. After detach the | ||
| /// isolate may already be gone; skipping is safer than replying on a dead channel. | ||
| private func deliverFlutterResult(_ result: @escaping FlutterResult, _ value: Any?) { |
There was a problem hiding this comment.
executeRpc's pre-dispatch guard builds a kPluginDetached FlutterError for the "called after
detach" case and routes it through deliverFlutterResult, but deliverFlutterResult itself
unconditionally no-ops whenever isEngineDetached is true:
private func deliverFlutterResult(_ result: @escaping FlutterResult, _ value: Any?) {
guard !isEngineDetached else { return }
result(value)
}Since this call site is only reached when isEngineDetached == true, the constructed
FlutterError is always discarded and result(...) is never invoked — the pending Dart Future
never completes. Either call result(...) directly for this one case, or give
deliverFlutterResult a bypass for intentional post-detach replies.
References:
There was a problem hiding this comment.
Good catch. The pre-dispatch PLUGIN_DETACHED path now calls result(...) directly on iOS (instead of routing through deliverFlutterResult). Added the same synchronous entry guard on Android for parity; async completions still drop in deliverFlutterResult / deliverRpcResult.
| attachment.methodChannel.setMethodCallHandler { call, result -> | ||
| handleMethodCall(attachment, call, result) | ||
| } | ||
| synchronized(attachmentsLock) { |
There was a problem hiding this comment.
onAttachedToEngine disposes a stale attachment while holding attachmentsLock
(attachments.remove(binding)?.dispose()), and dispose() calls
connectorWrapper?.stopObservingTransactions() — a call into the billing/Purchase Connector
client. onDetachedFromEngine correctly avoids this by removing from the map inside the lock and
disposing outside it; onAttachedToEngine doesn't follow the same pattern, so a slow/blocking
billing-client call on this defensive "re-attach with an already-registered binding" path could
stall onAttachedToEngine/onDetachedFromEngine for every other concurrently-active engine.
// Fix: mirror onDetachedFromEngine — remove under lock, dispose after releasing it
val stale = synchronized(attachmentsLock) {
val previous = attachments.remove(binding)
attachments[binding] = attachment
previous
}
stale?.dispose()References:
There was a problem hiding this comment.
Valid — onAttachedToEngine was disposing a stale attachment while holding attachmentsLock, unlike onDetachedFromEngine. Updated to remove/replace under the lock and call dispose() only after releasing it, so a slow stopObservingTransactions() on the defensive re-attach path cannot block lifecycle for other engines.
| return _invokeVoidRpc( | ||
| 'init', | ||
| _isIOS ? {'devKey': devKey, 'appId': appId} : {'devKey': devKey}, | ||
| ); |
There was a problem hiding this comment.
registerConversionListener (and the deep-link/session-ready registrars below it) writes the
app's callbacks into _listeners before dispatching the RPC that registers the native listener.
If _invokeVoidRpc('registerConversionListener') throws, the method rethrows to the caller, but
_listeners is left holding callbacks for an event native never actually registered — Dart
believes it's listening, native does not, and there's no rollback.
Future<void> registerConversionListener({...}) {
_ensureEventsSubscribed();
_listeners.on(EVENT_CONVERSION_DATA_SUCCESS, (event) => onSuccess(event.data));
_listeners.on(EVENT_CONVERSION_DATA_FAIL, (event) => onFailure?.call(event.data));
return _invokeVoidRpc('registerConversionListener'); // no rollback if this throws
}Roll back (_listeners.off(...))
in a catch.
References:
There was a problem hiding this comment.
Good catch — the three register*Listener methods now roll back their Dart callback slots with _listeners.off(...) if the native registration RPC throws, so Dart state cannot outlive a failed native registration.
| void dispatch(_AppsFlyerEvent event) { | ||
| final callback = _callbacks[event.name]; | ||
| if (callback != null) { | ||
| callback(event); |
There was a problem hiding this comment.
dispatch() (and _replay() at line 105) invoke the app's registered callback directly with no
error isolation: callback(event). If a host app's callback throws, the exception propagates out
of the plugin's single af-events handler into an uncaught zone error — and inside _replay,
since matching events were already removed from _pending via removeWhere before the delivery
loop runs, any events still queued behind the one that threw are lost for good, not just skipped
once.
void _invoke(void Function(_AppsFlyerEvent) callback, _AppsFlyerEvent event) {
try {
callback(event);
} catch (error, stack) {
debugPrint('AppsFlyer: listener for ${event.name} threw: $error\n$stack');
}
}References:
There was a problem hiding this comment.
Valid — dispatch() and _replay() now invoke app callbacks through a small try/catch wrapper that logs and continues, so a throwing listener cannot abort the af-events handler or drop the rest of a replay batch.
| // no dependency on `initialize`. It must run before `registerSessionReadyListener`, which | ||
| // Dart registers after `init()` — forwarding here satisfies that earlier than caching did. | ||
| executeJson(forMethod: "handleLaunchOptions", | ||
| params: ["launchOptions": jsonSafeOptions]) { _, _ in } |
There was a problem hiding this comment.
handleLaunchOptions is not a Dart-facing API — no Dart method ever calls it. It exists purely so
application(_:didFinishLaunchingWithOptions:), a legacy AppDelegate lifecycle
callback the SDK needs, can tell the native SDK about launch options. Routing a call that
never originates from Dart through executeJson/AFRPCBridge — full JSON envelope
serialization, the generic RPC dispatch path, SERIALIZATION_ERROR/FlutterError mapping meant
for actual Dart↔native calls — is the wrong mechanism for a native-to-native lifecycle hop. It
should call the underlying AppsFlyerRPC lifecycle API directly instead, which would also make the
discarded-completion concern below moot (a direct call has no RPC envelope to fail on).
This is in fact already the acknowledged direction for this exact class of call:
AppsFlyerAttribution.swift's own header comment says "Native Swift should call the typed
lifecycle API in AppsFlyerRPC directly once the upstream lifecycle-callback wrapper lands; this
class (and its JSON round-trip through AFRPCBridge) is then expected to be removed" — describing
continueUserActivity/handleOpenUrl, the same pattern handleLaunchOptions uses here just
inlined in AppsflyerSdkPlugin.swift instead of routed through AppsFlyerAttribution. Worth
confirming whether AppsFlyerRPC now exposes that typed API (this migration is already on RPC
7.0.12) and wiring handleLaunchOptions directly to it, dropping the RPC round-trip entirely.
Short of that: as long as this stays an RPC call, the discarded completion ({ _, _ in }) means
any failure (RPC dispatch error, malformed payload) is currently invisible in the field and in QA,
with nothing logged. AppsFlyerAttribution already gained logRpcFailureIfNeeded in this same
diff for comparable RPC failures; applying the same treatment here costs one line:
executeJson(forMethod: "handleLaunchOptions", params: ["launchOptions": jsonSafeOptions]) { _, error in
if let error = error {
os_log(.error, log: Self.log, "handleLaunchOptions failed: %{public}@", String(describing: error))
}
}References:
There was a problem hiding this comment.
Agreed. AppsFlyerRPCBridge still only exposes executeJson publicly, so this path remains interim. Added os_log on the handleLaunchOptions completion so serialization/RPC failures are visible in the field, matching the attribution lifecycle logging.
…ockingRpcExecutor
…n iOS and Android
Migrate Flutter plugin to AppsFlyer SDK 7 RPC architecture.
Route core APIs through af-api/af-events RPC bridges on Android and iOS, adopt the SDK 7 init + session-ready start model, remove SDK 6 APIs that no longer exist natively, bump native dependencies to 7.0.1, raise iOS minimum to 13.0, and reorganize public documentation.