fix(okhttp): keep the wrapped EventListener per Call

[markushi]

📜 Description

SentryOkHttpEventListener held the wrapped EventListener in a single mutable field that
callStart overwrote for each call. It is now kept in a per-Call map, the same pattern the class
already uses for eventMap. No public API change.

💡 Motivation and Context

OkHttp uses one listener instance for all calls, thus concurrent calls were all delegated to the
listener made for the call that started last. This breaks the EventListener.Factory contract and
loses the terminal callEnd/callFailed of every overlapping call.

• resolves: SentryOkHttpEventListener breaks EventListener.Factory contract #5967
• resolves: JAVA-695

💚 How did you test it?

Added unit tests.

📝 Checklist

• I added GH Issue ID & Linear ID
• I added tests to verify the changes.
• No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
• I updated the docs if needed.
• I updated the wizard if needed.
• Review from the native team if needed.
• No breaking change or entry added to the changelog.
• No breaking change for hybrid SDKs or communicated to hybrid SDKs.
• Public API changes reviewed by another Mobile SDK team member or implemented according to the develop docs spec.

🔮 Next steps

[linear-code]

JAVA-695

[sentry]

📲 Install Builds

Android

🔗 App Name	App ID	Version	Configuration
SDK Size	io.sentry.tests.size	8.54.0 (1)	release

⚙️ sentry-android Build Distribution Settings

[0xadam-brown]

Thanks for this 💯 !

One comment worth addressing; otherwise looking good.

[0xadam-brown]

Excellent! One tweak more to satisfy the EventListener.Factory contract, and I think we'll be there 🥇

[0xadam-brown]

We're close! (and thanks for the great updates)

We still need to preserve the contract of EventListener.Factory that ensures only one EventListener instance is produced per Call lifecycle. Ie, the listener returned by the factory for a given call needs to be the listener that captures i) all of that call's lifecycle and ii) no other call's lifecycle.

We've fixed (ii), but we're still violating (i) in the case of cancelation because we're creating an extra listener for early and late cancel() invocations.

Possible solution

Thoughts about using a weak per-call map for the wrapped listener instead? Something like a WeakHashMap<Call, EventListener> guarded by synchronized, with a getOrCreateOriginalEventListener(call) helper used by both callStart and canceled().

That'd^^ let us avoid removing entries on callEnd / callFailed, and completed calls would be gc'd as soon as the Call instance is unreachable.

[markushi]

Fixed in 728129d. You were right that creating an extra listener was the wrong trade — and a WeakHashMap turned out not to be usable here, for a reason our own code demonstrates.

Why not weak keys. WeakHashMap holds values by ordinary strong references, and its javadoc warns that a value which strongly refers to its own key prevents the key from being discarded. That is exactly our shape: a factory is handed the Call, so listeners that keep it are the normal case (our own RecordingListener(val ownCall: Call) does it). Worse, the sibling eventMap already has this cycle inside the SDK: SentryOkHttpEvent holds a Response, Response.exchange is an Exchange, and Exchange.call is the RealCall. So a weak map would stop collecting as soon as a response is recorded. It is also unsynchronized, and getTable() calls expungeStaleEntries(), so even reads would need the monitor on a path OkHttp explicitly allows to run concurrently.

What we do instead. OkHttp stores the listener on the call itself (RealCall:73, client.eventListenerFactory.create(this)), so its lifetime is the Call object's lifetime. Ours is the callStart()..callEnd() window. canceled() is the one event that escapes that window, so it is the one that needed handling — and Call.isExecuted() tells the two edges apart without any storage of our own:

• not executed — the cancel precedes callStart(). computeIfAbsent creates and binds the listener, and callStart() then reuses it, so one listener sees the whole lifecycle. It is self-cleaning: getResponseWithInterceptorChain does if (canceled) throw IOException("Canceled"), so a pre-canceled call that is later executed still runs callStart → callFailed and the entry is removed.
• executed, entry present — in flight, delegated to the bound listener.
• executed, entry absent — the terminal event already passed. Ignored. Call.cancel() is documented as "Requests that are already complete cannot be canceled", so there is nothing to report, and fabricating a second listener would both break the contract and leak the entry.

A stored "terminal" flag would have worked too, but it needs a per-Call marker that no later event can ever remove — the same unbounded growth, just with a smaller value behind a Call key. isExecuted() is that flag, maintained by OkHttp, for free.

Two supporting changes: getOrCreateEventListener uses ConcurrentHashMap.computeIfAbsent rather than a get-then-put, so a cancel racing callStart() cannot make the factory produce two listeners for one call; and the constructors that wrap a single EventListener now keep it in a field. That instance is shared across calls by definition — it is precisely what OkHttp's own EventListener.asFactory() does — so it exists independently of the window and receives every cancel, which restores the pre-PR behaviour you flagged with no leak and no contract question.

Residual gap, stated plainly: a call that is canceled before it starts and then never executed keeps its map entry, because no terminal event ever arrives. That is far narrower than the late-cancel leak it replaces, and bounding it would need the weak keys that do not work here.

[markushi]

Correction to my previous reply — 7b7f12e simplifies this further and removes the residual leak I disclosed there.

The Call.isExecuted() branch is gone. It bound a listener for a cancel that precedes callStart(), and that is exactly the case that can never be cleaned up: RealCall.cancel() fires canceled() unconditionally, but callStart is only reached from execute()/enqueue(), so newCall() → cancel() → never executed produces no terminal event at all. Any entry added there would retain the Call forever. (I checked the other paths — AsyncCall.run(), failRejected(), and cancel-while-queued all reach callDone() — so never-executed calls are the only such case.)

canceled() is now simply:

val originalEventListener = originalEventListenerMap[call] ?: fixedOriginalEventListener
originalEventListener?.canceled(call)

Out-of-window cancels are not delegated to factory-created listeners, and no listener is fabricated to receive them. The rationale is written into a comment on the method itself rather than left in this thread.

Little is actually lost: if the call is never executed there is no lifecycle to observe, and if it is executed after being canceled, OkHttp fails it with IOException("Canceled"), so the listener still learns about the cancellation through callFailed().

This gives your invariant without qualification — for any Call, the factory is invoked at most once, and that single listener sees the whole callStart()..terminal window and nothing from any other call. getOrCreateEventListener keeps computeIfAbsent so concurrent callbacks cannot produce two listeners for one call. Listeners passed as a single instance still receive every cancel, since they are shared by all calls by definition — the same as EventListener.asFactory().

Tests updated accordingly: cancel before callStart is not delegated and creates no listener, a call canceled before callStart still gets a single listener when it starts, cancel after the terminal event is ignored, cancel after a failed call is ignored. The mocked Call is no longer needed, so they run against real Call instances again.

[0xadam-brown]

Thanks for the new tests 💯

Bonus points if our cancellation tests can assert the stronger factory-contract invariant 👍

(Right now cancel before callStart is delegated and cancel after callEnd is delegated prove that some listener receives canceled(), rather than that a single listener receives all lifecycle callbacks.)

[markushi]

Done in 728129d. The cancellation tests now assert the single-listener invariant rather than "some listener got it".

The blocker was that these tests drive the listener by hand with client.newCall(...), so Call.isExecuted() is always false and the post-terminal branch was unreachable. Added a Fixture.mockCall(path, isExecuted) helper so each test states the call state it is exercising.

• cancel before callStart binds the listener that callStart then reuses — asserts fixture.listeners has size 1 and that the one listener receives canceled, callStart, dnsStart, callEnd in order.
• cancel after the terminal event is ignored — size 1, receiving exactly callStart, callEnd; no second listener is created and no stray canceled is delivered.
• cancel after a failed call is ignored — same, via callFailed.
• cancel during a call is delegated to the listener of that call — callStart, canceled, callFailed all on one listener.
• a single wrapped listener receives cancels outside of the call window — covers the fixed-instance constructors, which keep delegating cancels at any time.

The hasSize(1) assertions are the ones carrying the factory contract: they fail if we ever invoke the factory more than once for a call.

[markushi]

Follow-up: the cancellation tests changed again in 7b7f12e, along with the behaviour (see the other thread). The invariant they assert is unchanged and now stronger, because there is no case left where a second listener could appear.

• cancel before callStart is not delegated and creates no listener — fixture.listeners is empty.
• a call canceled before callStart still gets a single listener when it starts — size 1, receiving callStart, callFailed.
• cancel after the terminal event is ignored — size 1, receiving callStart, callEnd.
• cancel after a failed call is ignored — size 1, receiving callStart, callFailed.
• cancel during a call is delegated to the listener of that call — callStart, canceled, callFailed on one listener.
• a single wrapped listener receives cancels outside of the call window — the fixed-instance constructors keep delegating cancels at any time.

The mocked Call from my previous update is gone; these run against real Call instances again. Ten tests in the class, 94 in the module.

[cursor]

Cursor Bugbot has reviewed your changes and found 2 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

^{Reviewed by Cursor Bugbot for commit 728129d. Configure here.}