fix(okhttp): keep the wrapped EventListener per Call

CHANGELOG.md

@@ -5,6 +5,7 @@
 ### Fixes
 
 - Keep dropped tombstone and ANR events dropped, instead of reporting the same app exit again at every app start ([#6002](https://github.com/getsentry/sentry-java/pull/6002))
+- Keep the `EventListener` wrapped by `SentryOkHttpEventListener` per `Call` ([#6003](https://github.com/getsentry/sentry-java/pull/6003))
 - Apply `Sentry.withScope` and `Sentry.withIsolationScope` data to events captured inside the callback when `globalHubMode` is enabled ([#6004](https://github.com/getsentry/sentry-java/pull/6004))
   - `globalHubMode` is enabled by default on Android, where tags, extras, contexts and level set inside the callback were silently dropped
   - Scopes that are explicitly made current, e.g. via `Sentry.setCurrentScopes` or the `SentryContext` coroutine integration, are now also honoured when `globalHubMode` is enabled

sentry-okhttp/build.gradle.kts

@@ -38,6 +38,7 @@ dependencies {
   testImplementation(libs.mockito.inline)
   testImplementation(libs.okhttp)
   testImplementation(libs.okhttp.mockwebserver)
+  testImplementation(libs.google.truth)
 }
 
 buildConfig {

sentry-okhttp/src/main/java/io/sentry/okhttp/SentryOkHttpEventListener.kt

@@ -45,7 +45,12 @@ public open class SentryOkHttpEventListener(
   private val scopes: IScopes = ScopesAdapter.getInstance(),
   private val originalEventListenerCreator: ((call: Call) -> EventListener)? = null,
 ) : EventListener() {
-  private var originalEventListener: EventListener? = null
+  private val originalEventListenerMap: ConcurrentHashMap<Call, EventListener> = ConcurrentHashMap()
+
+  // Set only by the constructors that wrap a single EventListener instance. Such a listener is
+  // shared by every Call anyway, exactly like OkHttp's own EventListener.asFactory(), so it
+  // exists independently of the callStart()..callEnd() window and can always be delegated to.
+  private var fixedOriginalEventListener: EventListener? = null
 
   public companion object {
     internal const val PROXY_SELECT_EVENT = "http.client.proxy_select_ms"
@@ -65,7 +70,9 @@ public open class SentryOkHttpEventListener(
 
   public constructor(
     originalEventListener: EventListener
-  ) : this(ScopesAdapter.getInstance(), originalEventListenerCreator = { originalEventListener })
+  ) : this(ScopesAdapter.getInstance(), originalEventListenerCreator = { originalEventListener }) {
+    fixedOriginalEventListener = originalEventListener
+  }
 
   public constructor(
     originalEventListenerFactory: Factory
@@ -77,35 +84,41 @@ public open class SentryOkHttpEventListener(
   public constructor(
     scopes: IScopes = ScopesAdapter.getInstance(),
     originalEventListener: EventListener,
-  ) : this(scopes, originalEventListenerCreator = { originalEventListener })
+  ) : this(scopes, originalEventListenerCreator = { originalEventListener }) {
+    fixedOriginalEventListener = originalEventListener
+  }
 
   public constructor(
     scopes: IScopes = ScopesAdapter.getInstance(),
     originalEventListenerFactory: Factory,
   ) : this(scopes, originalEventListenerCreator = { originalEventListenerFactory.create(it) })
 
   override fun callStart(call: Call) {
-    originalEventListener = originalEventListenerCreator?.invoke(call)
+    // The EventListener.Factory contract binds a listener to a single call, so the wrapped
+    // listener is kept per call instead of in a field shared by all concurrent calls
+    val originalEventListener = getOrCreateEventListener(call)
     originalEventListener?.callStart(call)
     // If the wrapped EventListener is ours, we can just delegate the calls,
     // without creating other events that would create duplicates
-    if (canCreateEventSpan()) {
+    if (canCreateEventSpan(originalEventListener)) {
       eventMap[call] = SentryOkHttpEvent(scopes, call.request())
     }
   }
 
   override fun proxySelectStart(call: Call, url: HttpUrl) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.proxySelectStart(call, url)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
     okHttpEvent.onEventStart(PROXY_SELECT_EVENT)
   }
 
   override fun proxySelectEnd(call: Call, url: HttpUrl, proxies: List<Proxy>) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.proxySelectEnd(call, url, proxies)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
@@ -117,17 +130,19 @@ public open class SentryOkHttpEventListener(
   }
 
   override fun dnsStart(call: Call, domainName: String) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.dnsStart(call, domainName)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
     okHttpEvent.onEventStart(DNS_EVENT)
   }
 
   override fun dnsEnd(call: Call, domainName: String, inetAddressList: List<InetAddress>) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.dnsEnd(call, domainName, inetAddressList)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
@@ -140,26 +155,29 @@ public open class SentryOkHttpEventListener(
   }
 
   override fun connectStart(call: Call, inetSocketAddress: InetSocketAddress, proxy: Proxy) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.connectStart(call, inetSocketAddress, proxy)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
     okHttpEvent.onEventStart(CONNECT_EVENT)
   }
 
   override fun secureConnectStart(call: Call) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.secureConnectStart(call)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
     okHttpEvent.onEventStart(SECURE_CONNECT_EVENT)
   }
 
   override fun secureConnectEnd(call: Call, handshake: Handshake?) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.secureConnectEnd(call, handshake)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
@@ -172,8 +190,9 @@ public open class SentryOkHttpEventListener(
     proxy: Proxy,
     protocol: Protocol?,
   ) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.connectEnd(call, inetSocketAddress, proxy, protocol)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
@@ -188,8 +207,9 @@ public open class SentryOkHttpEventListener(
     protocol: Protocol?,
     ioe: IOException,
   ) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.connectFailed(call, inetSocketAddress, proxy, protocol, ioe)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
@@ -202,53 +222,59 @@ public open class SentryOkHttpEventListener(
   }
 
   override fun connectionAcquired(call: Call, connection: Connection) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.connectionAcquired(call, connection)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
     okHttpEvent.onEventStart(CONNECTION_EVENT)
   }
 
   override fun connectionReleased(call: Call, connection: Connection) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.connectionReleased(call, connection)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
     okHttpEvent.onEventFinish(CONNECTION_EVENT)
   }
 
   override fun requestHeadersStart(call: Call) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.requestHeadersStart(call)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
     okHttpEvent.onEventStart(REQUEST_HEADERS_EVENT)
   }
 
   override fun requestHeadersEnd(call: Call, request: Request) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.requestHeadersEnd(call, request)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
     okHttpEvent.onEventFinish(REQUEST_HEADERS_EVENT)
   }
 
   override fun requestBodyStart(call: Call) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.requestBodyStart(call)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
     okHttpEvent.onEventStart(REQUEST_BODY_EVENT)
   }
 
   override fun requestBodyEnd(call: Call, byteCount: Long) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.requestBodyEnd(call, byteCount)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
@@ -261,8 +287,9 @@ public open class SentryOkHttpEventListener(
   }
 
   override fun requestFailed(call: Call, ioe: IOException) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.requestFailed(call, ioe)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
@@ -282,17 +309,19 @@ public open class SentryOkHttpEventListener(
   }
 
   override fun responseHeadersStart(call: Call) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.responseHeadersStart(call)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
     okHttpEvent.onEventStart(RESPONSE_HEADERS_EVENT)
   }
 
   override fun responseHeadersEnd(call: Call, response: Response) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.responseHeadersEnd(call, response)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
@@ -307,17 +336,19 @@ public open class SentryOkHttpEventListener(
   }
 
   override fun responseBodyStart(call: Call) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.responseBodyStart(call)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
     okHttpEvent.onEventStart(RESPONSE_BODY_EVENT)
   }
 
   override fun responseBodyEnd(call: Call, byteCount: Long) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.responseBodyEnd(call, byteCount)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
@@ -330,8 +361,9 @@ public open class SentryOkHttpEventListener(
   }
 
   override fun responseFailed(call: Call, ioe: IOException) {
+    val originalEventListener = originalEventListenerMap[call]
     originalEventListener?.responseFailed(call, ioe)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap[call] ?: return
@@ -351,14 +383,15 @@ public open class SentryOkHttpEventListener(
   }
 
   override fun callEnd(call: Call) {
-    originalEventListener?.callEnd(call)
+    originalEventListenerMap.remove(call)?.callEnd(call)
     val okHttpEvent: SentryOkHttpEvent = eventMap.remove(call) ?: return
     okHttpEvent.finish()
   }
 
   override fun callFailed(call: Call, ioe: IOException) {
+    val originalEventListener = originalEventListenerMap.remove(call)
     originalEventListener?.callFailed(call, ioe)
-    if (!canCreateEventSpan()) {
+    if (!canCreateEventSpan(originalEventListener)) {
       return
     }
     val okHttpEvent: SentryOkHttpEvent = eventMap.remove(call) ?: return
@@ -370,26 +403,52 @@ public open class SentryOkHttpEventListener(
   }
 
   override fun canceled(call: Call) {
+    // canceled() is the only callback that is not part of the call window. OkHttp creates the
+    // listener in the Call constructor and keeps it for the whole lifetime of the Call, so it may
+    // deliver canceled() before callStart() and after callEnd()/callFailed(). We only keep the
+    // listener between callStart() and the terminal event, so out of that window there is nothing
+    // in originalEventListenerMap.
+    //
+    // We deliberately do not create a listener for such a cancel:
+    // - After the terminal event the call is over. Call.cancel() is documented as a no-op for a
+    //   request that is already complete, so there is nothing to report.
+    // - Before callStart() there may never be a call at all. A Call that is canceled and then
+    //   never executed gets no callEnd()/callFailed(), so an entry added here could never be
+    //   removed again and would leak the Call. If the Call is executed after all, OkHttp fails it
+    //   with IOException("Canceled") and the listener still learns about it through callFailed().
+    //
+    // Creating one would also hand the EventListener.Factory contract a second listener for a
+    // single Call. A listener kept in a field instead is shared by all calls by definition, the
+    // same as OkHttp's own EventListener.asFactory(), so it exists outside the window and always
+    // gets the event.
+    val originalEventListener = originalEventListenerMap[call] ?: fixedOriginalEventListener
     originalEventListener?.canceled(call)
   }
 
   override fun satisfactionFailure(call: Call, response: Response) {
-    originalEventListener?.satisfactionFailure(call, response)
+    originalEventListenerMap[call]?.satisfactionFailure(call, response)
   }
 
   override fun cacheHit(call: Call, response: Response) {
-    originalEventListener?.cacheHit(call, response)
+    originalEventListenerMap[call]?.cacheHit(call, response)
   }
 
   override fun cacheMiss(call: Call) {
-    originalEventListener?.cacheMiss(call)
+    originalEventListenerMap[call]?.cacheMiss(call)
   }
 
   override fun cacheConditionalHit(call: Call, cachedResponse: Response) {
-    originalEventListener?.cacheConditionalHit(call, cachedResponse)
+    originalEventListenerMap[call]?.cacheConditionalHit(call, cachedResponse)
+  }
+
+  // computeIfAbsent, so that concurrent callbacks cannot make the Factory produce two listeners
+  // for the same Call
+  private fun getOrCreateEventListener(call: Call): EventListener? {
+    val creator = originalEventListenerCreator ?: return null
+    return originalEventListenerMap.computeIfAbsent(call) { creator.invoke(it) }
   }
 
-  private fun canCreateEventSpan(): Boolean {
+  private fun canCreateEventSpan(originalEventListener: EventListener?): Boolean {
     // If the wrapped EventListener is ours, we shouldn't create spans, as the originalEventListener
     // already did it
     // In case SentryOkHttpEventListener from sentry-android-okhttp is used, the is check won't work