fix(okhttp): keep the wrapped EventListener per Call - #6003
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📲 Install BuildsAndroid
|
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Move the okhttp changelog entry into a new Unreleased section, as 8.54.0 was released on main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
0xadam-brown
left a comment
0xadam-brown
left a comment
There was a problem hiding this comment.
Thanks for this 💯 !
One comment worth addressing; otherwise looking good.
Keep both Unreleased changelog entries. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
bb23afd to
b508070
Compare
0xadam-brown
left a comment
0xadam-brown
left a comment
There was a problem hiding this comment.
Excellent! One tweak more to satisfy the EventListener.Factory contract, and I think we'll be there 🥇
| // callEnd()/callFailed(), so there is not always a listener bound to the call. Create one on | ||
| // the fly in that case, but do not put it in the map: nothing would remove it again, because | ||
| // a call that is canceled before it starts never gets a callEnd() or callFailed(). | ||
| val originalEventListener = |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| @Test | ||
| fun `cancel before callStart is delegated`() { |
There was a problem hiding this comment.
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.)
2 participants
📜 Description
SentryOkHttpEventListenerheld the wrappedEventListenerin a single mutable field thatcallStartoverwrote for each call. It is now kept in a per-Callmap, the same pattern the classalready 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.Factorycontract andloses the terminal
callEnd/callFailedof every overlapping call.💚 How did you test it?
Added unit tests.
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps