amplitude / amplitude/AmplitudeCore-Swift

Memory leak in RemoteConfigClient.init during SDK initialization

Aperta
#57 1 commento 0 reazioni 1 assegnatario Rivendicata da @sojingle Vedi su GitHub
bug Internal operations
Lingua principale
Swift
Stelle
3
Fork
2
Merge medio
13h 21m
PR unite (30g)
6

Descrizione

## Summary

`RemoteConfigClient.init` (and `subscribe`/`updateConfigs`) cause Instruments → Leaks to report a ~32-byte allocation (`swift_weakInit` / `formWeakReference`) that's never freed for the instance's lifetime. Reported in amplitude/AmplitudeCore-Swift#57, Amplitude-Swift#413, and amplitude/Amplitude-Swift#413 (now a duplicate of this).

It is **not a real leak** — the ~32-byte ObjC weak side-table entry is managed correctly by the runtime and would be freed when the instance dies. Because `RemoteConfigClient` is held for the SDK-instance lifetime it effectively never frees, and Instruments' conservative scanner can't trace the (encoded) pointer to it, so it reports reachable memory as leaked. The real annoyance is the noise in customers' leak reports.

## Root cause

`RemoteConfigClient` is an `actor` that subclasses `NSObject`. A `[weak self]` to an NSObject-rooted instance routes through the ObjC weak side table (the leaking 32-byte entry). The leak needs BOTH the NSObject root AND a weak self-capture — verified with a 2×2 (NSObject × weak-self) repro; removing either one eliminates it.

## Chosen fix (this ticket)

Split the type so the thing we weak-reference is native Swift, not NSObject:

* `RemoteConfigClient` becomes a thin `final class: NSObject, @unchecked Sendable` (no longer an `actor`) — a public facade holding `private let core: Core`, forwarding its (already `nonisolated`) public methods; its `deinit` tears down the core.
* `Core` is an internal pure-Swift `actor` holding all state/logic (callbacks, fetch tasks, `sendCallback`, fetch). Its tasks use `[weak self]` (weak `Core`) — native, so no ObjC side table and no leak — and capture specific values (e.g. `storage`) for work that must complete regardless of release.

Gets all four properties at once: no side-table leak (Core is native) · cancel-on-release preserved (Core deallocates when the shell releases it → `deinit` cancels in-flight fetches) · serialized callback delivery (Core is an actor; `sendCallback` stays isolated → no data race) · `NSObject` shell preserved. Removes the root-cause `actor: NSObject` anti-pattern; reference graph stays acyclic (shell → core one-way; core never references the shell; core's tasks → core via native weak).

## Alternatives considered

* **Drop** `NSObject` (cleanest in isolation) — rejected: SessionReplay's public `@objc convenience init(... remoteConfigClient: RemoteConfigClient? ...)` requires `RemoteConfigClient` to stay Obj-C-representable.
* **Minimal fix: keep** `NSObject`**, replace all** `[weak self]` **with strong captures** — explored in PR amplitude/AmplitudeCore-Swift#84, **not being merged**: it removes the leak but loses `deinit` cancellation (a released client's in-flight fetch runs to completion instead of cancelling). We'd rather do the proper refactor than ship that trade-off.
* **Explicit cancel API / manual ref-counting** — rejected as fragile: re-implements ARC, and the client is shared (no single owner can correctly decide when to cancel; the right signal is the shell's `deinit`, which is what the split restores).

## Priority

Low / not urgent — benign Instruments false positive, no interim fix is shipping. This is the proper fix for when it's scheduled.

## Acceptance criteria

- [ ] `RemoteConfigClient` is no longer an `actor`; logic lives in an internal `Core` actor; no `[weak self]` to an NSObject-rooted instance anywhere.
- [ ] Instruments shows 0 side-table leaks on init/subscribe/updateConfigs (retained-instance harness).
- [ ] Releasing a client cancels its in-flight fetch (covered by a test).
- [ ] Callback delivery remains serialized; existing RemoteConfigClient tests pass.
- [ ] SessionReplay still compiles/links (its `@objc` init takes `RemoteConfigClient?`).

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.