diff --git a/dd-sdk-android-core/api/apiSurface b/dd-sdk-android-core/api/apiSurface index aae834fcc6..f6f5804e7c 100644 --- a/dd-sdk-android-core/api/apiSurface +++ b/dd-sdk-android-core/api/apiSurface @@ -130,7 +130,7 @@ interface com.datadog.android.api.feature.FeatureScope val dataStore: com.datadog.android.api.storage.datastore.DataStoreHandler fun withWriteContext(Set = emptySet(), (com.datadog.android.api.context.DatadogContext) -> Unit) fun withContext(Set = emptySet(), (com.datadog.android.api.context.DatadogContext) -> Unit) - fun getWriteContextSync(Set = emptySet()): Pair? + fun withWriteContextSync(Set = emptySet(), (com.datadog.android.api.context.DatadogContext) -> Unit): Boolean fun sendEvent(Any) fun unwrap(): T typealias EventWriteScope = ((com.datadog.android.api.storage.EventBatchWriter) -> Unit) -> Unit diff --git a/dd-sdk-android-core/api/dd-sdk-android-core.api b/dd-sdk-android-core/api/dd-sdk-android-core.api index aa0c2252e4..c0801fc139 100644 --- a/dd-sdk-android-core/api/dd-sdk-android-core.api +++ b/dd-sdk-android-core/api/dd-sdk-android-core.api @@ -414,17 +414,17 @@ public abstract interface class com/datadog/android/api/feature/FeatureEventRece public abstract interface class com/datadog/android/api/feature/FeatureScope { public abstract fun getDataStore ()Lcom/datadog/android/api/storage/datastore/DataStoreHandler; - public abstract fun getWriteContextSync (Ljava/util/Set;)Lkotlin/Pair; public abstract fun sendEvent (Ljava/lang/Object;)V public abstract fun unwrap ()Lcom/datadog/android/api/feature/Feature; public abstract fun withContext (Ljava/util/Set;Lkotlin/jvm/functions/Function1;)V public abstract fun withWriteContext (Ljava/util/Set;Lkotlin/jvm/functions/Function2;)V + public abstract fun withWriteContextSync (Ljava/util/Set;Lkotlin/jvm/functions/Function2;)Z } public final class com/datadog/android/api/feature/FeatureScope$DefaultImpls { - public static synthetic fun getWriteContextSync$default (Lcom/datadog/android/api/feature/FeatureScope;Ljava/util/Set;ILjava/lang/Object;)Lkotlin/Pair; public static synthetic fun withContext$default (Lcom/datadog/android/api/feature/FeatureScope;Ljava/util/Set;Lkotlin/jvm/functions/Function1;ILjava/lang/Object;)V public static synthetic fun withWriteContext$default (Lcom/datadog/android/api/feature/FeatureScope;Ljava/util/Set;Lkotlin/jvm/functions/Function2;ILjava/lang/Object;)V + public static synthetic fun withWriteContextSync$default (Lcom/datadog/android/api/feature/FeatureScope;Ljava/util/Set;Lkotlin/jvm/functions/Function2;ILjava/lang/Object;)Z } public final class com/datadog/android/api/feature/FeatureScopeExtKt { diff --git a/dd-sdk-android-core/src/main/kotlin/com/datadog/android/api/feature/FeatureScope.kt b/dd-sdk-android-core/src/main/kotlin/com/datadog/android/api/feature/FeatureScope.kt index 9faa4253f9..1977e1afb6 100644 --- a/dd-sdk-android-core/src/main/kotlin/com/datadog/android/api/feature/FeatureScope.kt +++ b/dd-sdk-android-core/src/main/kotlin/com/datadog/android/api/feature/FeatureScope.kt @@ -52,19 +52,31 @@ interface FeatureScope { callback: (datadogContext: DatadogContext) -> Unit ) - // TODO RUM-9852 Implement better passthrough mechanism for the JVM crash scenario /** - * Same as [withWriteContext] but will be executed in the blocking manner. + * Same as [withWriteContext], but blocks the calling thread until [callback] has returned. + * + * The callback still runs on the context processing worker thread, so the calling thread must not + * hold any lock that [callback] may need, otherwise the two will deadlock. * * @param withFeatureContexts Feature contexts ([DatadogContext.featuresContext] property) to include * in the [DatadogContext] provided. The value should be the feature names as declared by [Feature.name]. * Default is empty, meaning that no feature contexts will be included. + * @param callback an operation called with an up-to-date [DatadogContext] + * and an [EventWriteScope]. Callback will be executed on a single context processing worker thread. Execution of + * [EventWriteScope] will be done on a worker thread from I/O pool. + * [DatadogContext] is a snapshot taken when [callback] starts executing, which is once every context + * operation scheduled before this call has completed. + * @return `true` if [callback] was executed, `false` if it could not be, for example because the + * SDK core is not initialized or the operation could not be scheduled. * * **NOTE**: This API is for the internal use only and is not guaranteed to be stable. */ @AnyThread @InternalApi - fun getWriteContextSync(withFeatureContexts: Set = emptySet()): Pair? + fun withWriteContextSync( + withFeatureContexts: Set = emptySet(), + callback: (datadogContext: DatadogContext, write: EventWriteScope) -> Unit + ): Boolean /** * Send event to a given feature. It will be sent in a synchronous way. diff --git a/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/SdkFeature.kt b/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/SdkFeature.kt index 67a5bee33c..78fb0c1179 100644 --- a/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/SdkFeature.kt +++ b/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/SdkFeature.kt @@ -215,22 +215,24 @@ internal class SdkFeature( ) } - override fun getWriteContextSync( - withFeatureContexts: Set - ): Pair? { - val operationName = "getWriteContextSync-${wrappedFeature.name}" + override fun withWriteContextSync( + withFeatureContexts: Set, + callback: (DatadogContext, EventWriteScope) -> Unit + ): Boolean { + val operationName = "withWriteContextSync-${wrappedFeature.name}" return coreFeature.contextExecutorService .submitSafe( operationName, internalLogger, Callable { - if (!coreFeature.initialized.get()) return@Callable null + if (!coreFeature.initialized.get()) return@Callable false val context = contextProvider.getContext(withFeatureContexts) val eventBatchWriteScope = storage.getEventWriteScope(context) - context to eventBatchWriteScope + callback(context, eventBatchWriteScope) + true } ) - .getSafe(operationName, internalLogger) + .getSafe(operationName, internalLogger) ?: false } override fun sendEvent(event: Any) { diff --git a/dd-sdk-android-core/src/test/kotlin/com/datadog/android/core/internal/SdkFeatureTest.kt b/dd-sdk-android-core/src/test/kotlin/com/datadog/android/core/internal/SdkFeatureTest.kt index 28926229f5..88e4258edb 100644 --- a/dd-sdk-android-core/src/test/kotlin/com/datadog/android/core/internal/SdkFeatureTest.kt +++ b/dd-sdk-android-core/src/test/kotlin/com/datadog/android/core/internal/SdkFeatureTest.kt @@ -565,18 +565,19 @@ internal class SdkFeatureTest { } @Test - fun `M provide write context W getWriteContextSync()`( + fun `M provide write context W withWriteContextSync(callback)`( @Forgery fakeContext: DatadogContext, @StringForgery fakeWithFeatureContexts: Set, @Mock mockEventWriteScope: EventWriteScope ) { // Given testedFeature.storage = mockStorage + val callback = mock<(DatadogContext, EventWriteScope) -> Unit>() whenever(mockContextProvider.getContext(fakeWithFeatureContexts)) doReturn fakeContext - whenever(coreFeature.mockInstance.contextExecutorService.submit(any>())) doAnswer { - val callable = it.getArgument>>(0) - mock>().apply { - whenever(get()) doAnswer { callable.call() } + whenever(coreFeature.mockInstance.contextExecutorService.submit(any>())) doAnswer { + val result = it.getArgument>(0).call() + mock>().apply { + whenever(get()) doReturn result } } @@ -585,77 +586,107 @@ internal class SdkFeatureTest { ) doReturn mockEventWriteScope // When - val writeContext = testedFeature.getWriteContextSync(fakeWithFeatureContexts) + val result = testedFeature.withWriteContextSync(fakeWithFeatureContexts, callback = callback) // Then - checkNotNull(writeContext) - assertThat(writeContext.first).isEqualTo(fakeContext) - assertThat(writeContext.second).isEqualTo(mockEventWriteScope) + verify(callback).invoke(fakeContext, mockEventWriteScope) + assertThat(result).isTrue() } @Test - fun `M provide null write context W getWriteContextSync() { task rejected }`( + fun `M wait for the callback W withWriteContextSync(callback)`( @Forgery fakeContext: DatadogContext, + @StringForgery fakeWithFeatureContexts: Set, @Mock mockEventWriteScope: EventWriteScope ) { // Given testedFeature.storage = mockStorage - whenever( - coreFeature.mockInstance.contextExecutorService.submit(any>()) - ) doThrow RejectedExecutionException() + whenever(mockContextProvider.getContext(fakeWithFeatureContexts)) doReturn fakeContext + whenever(mockStorage.getEventWriteScope(fakeContext)) doReturn mockEventWriteScope + // the submitted task only runs when the future is awaited, so the callback can only have been + // invoked if withWriteContextSync blocked on it + whenever(coreFeature.mockInstance.contextExecutorService.submit(any>())) doAnswer { + val submittedTask = it.getArgument>(0) + mock>().apply { + whenever(get()) doAnswer { submittedTask.call() } + } + } + var callbackInvoked = false + + // When + testedFeature.withWriteContextSync(fakeWithFeatureContexts) { _, _ -> callbackInvoked = true } + // Then + assertThat(callbackInvoked).isTrue() + } + + @Test + fun `M not provide write context W withWriteContextSync(callback) { task rejected }`( + @StringForgery fakeWithFeatureContexts: Set + ) { + // Given + testedFeature.storage = mockStorage + val callback = mock<(DatadogContext, EventWriteScope) -> Unit>() whenever( - mockStorage.getEventWriteScope(fakeContext) - ) doReturn mockEventWriteScope + coreFeature.mockInstance.contextExecutorService.submit(any>()) + ) doThrow RejectedExecutionException() // When - val writeContext = testedFeature.getWriteContextSync() + val result = testedFeature.withWriteContextSync(fakeWithFeatureContexts, callback = callback) // Then - assertThat(writeContext).isNull() + verifyNoInteractions(callback, mockContextProvider, mockStorage) + assertThat(result).isFalse() } @Test - fun `M provide null write context W getWriteContextSync() { failed to get task result }`( - @Forgery fakeContext: DatadogContext, - @Mock mockEventWriteScope: EventWriteScope, + fun `M not throw W withWriteContextSync(callback) { failed to get task result }`( + @StringForgery fakeWithFeatureContexts: Set, forge: Forge ) { // Given testedFeature.storage = mockStorage + val callback = mock<(DatadogContext, EventWriteScope) -> Unit>() val throwable = forge.anElementFrom( CancellationException(), ExecutionException(forge.aThrowable()), InterruptedException() ) - whenever(coreFeature.mockInstance.contextExecutorService.submit(any>())) doAnswer { - mock>().apply { + whenever(coreFeature.mockInstance.contextExecutorService.submit(any>())) doAnswer { + mock>().apply { whenever(get()) doThrow throwable } } - whenever( - mockStorage.getEventWriteScope(fakeContext) - ) doReturn mockEventWriteScope - // When - val writeContext = testedFeature.getWriteContextSync() + val result = testedFeature.withWriteContextSync(fakeWithFeatureContexts, callback = callback) // Then - assertThat(writeContext).isNull() + verifyNoInteractions(callback) + assertThat(result).isFalse() } @Test - fun `M provide null write context W getWriteContextSync() { CoreFeature is not initialized }`() { + fun `M not provide write context W withWriteContextSync(callback) { CoreFeature is not initialized }`( + @StringForgery fakeWithFeatureContexts: Set + ) { // Given + testedFeature.storage = mockStorage + val callback = mock<(DatadogContext, EventWriteScope) -> Unit>() whenever(coreFeature.mockInstance.initialized) doReturn AtomicBoolean(false) + whenever(coreFeature.mockInstance.contextExecutorService.submit(any>())) doAnswer { + val taskResult = it.getArgument>(0).call() + mock>().apply { + whenever(get()) doReturn taskResult + } + } // When - val writeContext = testedFeature.getWriteContextSync() + val result = testedFeature.withWriteContextSync(fakeWithFeatureContexts, callback = callback) // Then - assertThat(writeContext).isNull() - verifyNoInteractions(mockContextProvider, mockStorage) + verifyNoInteractions(callback, mockContextProvider, mockStorage) + assertThat(result).isFalse() } @Test diff --git a/features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/monitor/DatadogRumMonitor.kt b/features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/monitor/DatadogRumMonitor.kt index 4a3ea7a3ee..28a9f1b0c1 100644 --- a/features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/monitor/DatadogRumMonitor.kt +++ b/features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/monitor/DatadogRumMonitor.kt @@ -919,26 +919,26 @@ internal class DatadogRumMonitor( internal fun handleEvent(event: RumRawEvent) { if (event is RumRawEvent.AddError && event.isFatal) { - synchronized(rootScope) { - // TODO RUM-9852 Implement better passthrough mechanism for the JVM crash scenario - val writeContext = sdkCore.getFeature(Feature.RUM_FEATURE_NAME) - ?.getWriteContextSync(withFeatureContexts = setOf(Feature.SESSION_REPLAY_FEATURE_NAME)) - if (writeContext != null) { - val (datadogContext, eventWriteScope) = writeContext - @Suppress("ThreadSafety") // Crash handling, can't delegate to another thread - rootScope.handleEvent(event, datadogContext, eventWriteScope, writer) - val rumContext = currentRumContext() - sdkCore.updateFeatureContext(Feature.RUM_FEATURE_NAME) { - it.clear() - rumContext?.toMap()?.let(it::putAll) + val handled = sdkCore.getFeature(Feature.RUM_FEATURE_NAME) + ?.withWriteContextSync( + withFeatureContexts = setOf(Feature.SESSION_REPLAY_FEATURE_NAME) + ) { datadogContext, eventWriteScope -> + synchronized(rootScope) { + @Suppress("ThreadSafety") // Crash handling, can't delegate to another thread + rootScope.handleEvent(event, datadogContext, eventWriteScope, writer) + val rumContext = currentRumContext() + sdkCore.updateFeatureContext(Feature.RUM_FEATURE_NAME, useContextThread = false) { + it.clear() + rumContext?.toMap()?.let(it::putAll) + } } - } else { - sdkCore.internalLogger.log( - InternalLogger.Level.WARN, - InternalLogger.Target.USER, - { CANNOT_WRITE_CRASH_WRITE_CONTEXT_IS_NOT_AVAILABLE } - ) } + if (handled != true) { + sdkCore.internalLogger.log( + InternalLogger.Level.WARN, + InternalLogger.Target.USER, + { CANNOT_WRITE_CRASH_WRITE_CONTEXT_IS_NOT_AVAILABLE } + ) } } else if (event is RumRawEvent.TelemetryEventWrapper) { telemetryEventHandler.handleEvent(event, writer) diff --git a/features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewManagerScopeTest.kt b/features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewManagerScopeTest.kt index 4490270637..5b4797da66 100644 --- a/features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewManagerScopeTest.kt +++ b/features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewManagerScopeTest.kt @@ -162,6 +162,7 @@ internal class RumViewManagerScopeTest { fakeSampleRate = forge.aFloat(min = 0.0f, max = 100.0f) whenever(mockSdkCore.time) doReturn fakeTime + whenever(mockSdkCore.timeProvider) doReturn mock() whenever(mockParentScope.getRumContext()) doReturn fakeParentContext whenever(mockChildScope.handleEvent(any(), any(), any(), any())) doReturn mockChildScope diff --git a/features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/monitor/DatadogRumMonitorTest.kt b/features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/monitor/DatadogRumMonitorTest.kt index 643d961a42..6a5ccacc06 100644 --- a/features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/monitor/DatadogRumMonitorTest.kt +++ b/features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/monitor/DatadogRumMonitorTest.kt @@ -237,6 +237,10 @@ internal class DatadogRumMonitorTest { whenever(mockExecutorService.execute(any())) doAnswer { it.getArgument(0).run() } + whenever(mockExecutorService.submit(any())) doAnswer { + it.getArgument(0).run() + mock>() + } whenever(mockExecutorService.submit(any>())) doAnswer { val rumContext = it.getArgument>(0).call() mock>().apply { whenever(get()) doReturn rumContext } @@ -269,8 +273,15 @@ internal class DatadogRumMonitorTest { callback.invoke(fakeDatadogContext, mockEventWriteScope) } whenever( - mockRumFeatureScope.getWriteContextSync(setOf(Feature.SESSION_REPLAY_FEATURE_NAME)) - ) doReturn (fakeDatadogContext to mockEventWriteScope) + mockRumFeatureScope.withWriteContextSync( + eq(setOf(Feature.SESSION_REPLAY_FEATURE_NAME)), + any() + ) + ) doAnswer { + val callback = it.getArgument<(DatadogContext, EventWriteScope) -> Unit>(it.arguments.lastIndex) + callback.invoke(fakeDatadogContext, mockEventWriteScope) + true + } fakeAttributes = forge.exhaustiveAttributes() @@ -1351,16 +1362,14 @@ internal class DatadogRumMonitorTest { } @Test - fun `M delegate event to rootScope on current thread W addCrash()`( + fun `M delegate event to rootScope W addCrash()`( @StringForgery message: String, @Forgery source: RumErrorSource, @Forgery throwable: Throwable, forge: Forge ) { // Given - whenever( - mockRumFeatureScope.getWriteContextSync(setOf(Feature.SESSION_REPLAY_FEATURE_NAME)) - ) doReturn (fakeDatadogContext to mockEventWriteScope) + // the fatal event is handled without the RUM pipeline, so a drained executor changes nothing testedMonitor.drainExecutorService() val now = System.nanoTime() val appStartTimeNs = forge.aLong(min = 0L, max = now) @@ -1398,7 +1407,12 @@ internal class DatadogRumMonitorTest { @Forgery throwable: Throwable ) { // Given - whenever(mockRumFeatureScope.getWriteContextSync(setOf(Feature.SESSION_REPLAY_FEATURE_NAME))) doReturn null + whenever( + mockRumFeatureScope.withWriteContextSync( + eq(setOf(Feature.SESSION_REPLAY_FEATURE_NAME)), + any() + ) + ) doReturn false // When testedMonitor.addCrash(message, source, throwable, threads = emptyList()) @@ -1412,6 +1426,59 @@ internal class DatadogRumMonitorTest { verifyNoInteractions(mockApplicationScope, mockWriter) } + @Test + fun `M not hold rootScope lock W addCrash() { fatal error }`( + @StringForgery message: String, + @Forgery source: RumErrorSource, + @Forgery throwable: Throwable + ) { + // Given + var rootScopeHeldWhileWaiting = true + whenever( + mockRumFeatureScope.withWriteContextSync( + eq(setOf(Feature.SESSION_REPLAY_FEATURE_NAME)), + any() + ) + ) doAnswer { + rootScopeHeldWhileWaiting = Thread.holdsLock(testedMonitor.rootScope) + val callback = it.getArgument<(DatadogContext, EventWriteScope) -> Unit>(it.arguments.lastIndex) + callback.invoke(fakeDatadogContext, mockEventWriteScope) + true + } + + // When + testedMonitor.addCrash(message, source, throwable, threads = emptyList()) + + // Then + assertThat(rootScopeHeldWhileWaiting) + .withFailMessage( + "The crashing thread must not hold the rootScope lock while waiting on the context" + + " thread — lock-ordering inversion causes a deadlock (RUM-17619)" + ) + .isFalse() + } + + @Test + fun `M not delegate to the RUM executor W addCrash() { fatal error }`( + @StringForgery message: String, + @Forgery source: RumErrorSource, + @Forgery throwable: Throwable + ) { + // When + testedMonitor.addCrash(message, source, throwable, threads = emptyList()) + + // Then + // the crash is handled on the context thread: the extra hop would only add latency and expose the + // event to the RUM executor back-pressure, and ordering is already guaranteed by the context thread + verify(mockExecutorService, never()).submit(any>()) + verify(mockApplicationScope).handleEvent( + any(), + same(fakeDatadogContext), + same(mockEventWriteScope), + same(mockWriter) + ) + } + @Test fun `M delegate event to rootScope W resetSession()`() { // When @@ -2927,7 +2994,6 @@ internal class DatadogRumMonitorTest { ) { // Given val mockFeatureScope = mock() - whenever(mockFeatureScope.getWriteContextSync(setOf(Feature.SESSION_REPLAY_FEATURE_NAME))) doReturn null whenever(mockSdkCore.getFeature(Feature.RUM_FEATURE_NAME)) doReturn mockFeatureScope whenever(mockExecutorService.submit(any>())) doAnswer { mock>().apply { whenever(get()) doReturn null } diff --git a/reliability/stub-core/src/main/kotlin/com/datadog/android/core/stub/StubFeatureScope.kt b/reliability/stub-core/src/main/kotlin/com/datadog/android/core/stub/StubFeatureScope.kt index 59c0a67249..d34620bc7d 100644 --- a/reliability/stub-core/src/main/kotlin/com/datadog/android/core/stub/StubFeatureScope.kt +++ b/reliability/stub-core/src/main/kotlin/com/datadog/android/core/stub/StubFeatureScope.kt @@ -71,8 +71,15 @@ internal class StubFeatureScope( callback(datadogContextProvider()) } - override fun getWriteContextSync(withFeatureContexts: Set): Pair? { - return datadogContextProvider() to { it.invoke(eventBatchWriter) } + override fun withWriteContextSync( + withFeatureContexts: Set, + callback: (DatadogContext, EventWriteScope) -> Unit + ): Boolean { + callback( + datadogContextProvider(), + { it.invoke(eventBatchWriter) } + ) + return true } override fun sendEvent(event: Any) {