RUM-1462 Fix thread safety warnings#2056
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2056 +/- ##
===========================================
- Coverage 83.16% 82.99% -0.17%
===========================================
Files 494 494
Lines 17735 17727 -8
Branches 2684 2682 -2
===========================================
- Hits 14748 14712 -36
- Misses 2252 2279 +27
- Partials 735 736 +1
|
ef5f721 to
43d5f42
Compare
43d5f42 to
d727a7e
Compare
| coreFeature.deleteLastViewEvent() | ||
| @Suppress("ThreadSafety") // removal of the data is done in synchronous manner | ||
| coreFeature.deleteLastFatalAnrSent() | ||
| getPersistenceExecutorService().submitSafe("Clear all data", internalLogger) { |
There was a problem hiding this comment.
isn't this going to create issues if we expect this method to return in the same stack immediately ?
There was a problem hiding this comment.
There's no such expectations, and it would mostly create issues if called from the main thread on a running app.
| synchronized(lockedBatches) { | ||
| lockedBatches.forEach { | ||
| deleteBatch(it, RemovalReason.Flushed) | ||
| executorService.submitSafe("ConsentAwareStorage.dropAll", internalLogger) { |
There was a problem hiding this comment.
same question as above ? What if we call this from an unit test tearDown or setup method ?
There was a problem hiding this comment.
Important
Honestly, if we need to call this from a test, we can adapt the test to this call being async. We should not make things synchronous at runtime because of testing constraints.
| testedCore.coreFeature = mockCoreFeature | ||
| val mockExecutorService: FlushableExecutorService = mock() | ||
| whenever(mockCoreFeature.persistenceExecutorService) doReturn mockExecutorService | ||
| whenever(mockExecutorService.submit(any())) doAnswer { |
There was a problem hiding this comment.
I see so this is because we've made those methods async ?
0xnm
left a comment
0xnm
left a comment
There was a problem hiding this comment.
lgtm, but I'm curious about the behaviour of Datadog.clearAllData in iOS SDK.
| * [DatadogContext] will have a state created at the moment this method is called, before the | ||
| * thread switch for the callback invocation. | ||
| */ | ||
| @AnyThread |
There was a problem hiding this comment.
should we make this one as implicit in the detector in order to reduce the number of sites where we should put it? like if method is not annotated, then it is AnyThread by default.
There was a problem hiding this comment.
We could, I mostly added it to make it explicit while checking all thread safety warnings
| } | ||
| // TODO RUM-1462 address Thread safety | ||
| @Suppress("ThreadSafety") // removal of the data is done in synchronous manner | ||
| coreFeature.deleteLastViewEvent() | ||
| @Suppress("ThreadSafety") // removal of the data is done in synchronous manner | ||
| coreFeature.deleteLastFatalAnrSent() | ||
| getPersistenceExecutorService().submitSafe("Clear all data", internalLogger) { | ||
| coreFeature.deleteLastViewEvent() | ||
| coreFeature.deleteLastFatalAnrSent() | ||
| } | ||
| } |
There was a problem hiding this comment.
I think it is done in sync way by design, so that the call on the caller side is blocking (even though it may happen on the main thread) and the same ideology as with features.values.forEach { it.clearAllData() } above. Is it blocking in iOS SDK?
There was a problem hiding this comment.
Just checked, on iOS the call is async
| } | ||
|
|
||
| private var parentFunGroup: ThreadGroup = ThreadGroup.UNKNOWN | ||
| private val parentFunGroupStack: MutableList<ThreadGroup> = mutableListOf(ThreadGroup.UNKNOWN) |
There was a problem hiding this comment.
minor: since we are treating it like a stack, we can use Stack type here (or Deque -> LinkedList) which will allows us simply use push/pop/peek methods instead of add(index, ...) and removeAt(index, ...)
| telemetryEventHandler.handleEvent(event, writer) | ||
| // If an issue arise, use the unbound logger to prevent a cycle that would lead | ||
| // to a stack overflow | ||
| executorService.submitSafe("Telemetry event handling", InternalLogger.UNBOUND) { |
There was a problem hiding this comment.
does it make sense to create a dedicated thread for the telemetry processing? RUM monitor thread may already be busy and we don't need processing thread to be especially this one.
on another hand, even though TelemetryEventHandler.handleEvent is annotated with @WorkerThread, in reality this annotation may be redundant, because the amount of work done there before the switch to withWriteContext is very small.
There was a problem hiding this comment.
That's a good point, I'll change that.
4 participants
What does this PR do?
A rule we used for a while now is using the Thread annotation (e.g.:
@UIThread,@WorkerThread, …) to ensure operations that should happen in a specific context didn't cross thread expectations.Unfortunately we had a large number of
@Suppressannotations and thread boundaries crossings in our code base.This PR provides two things :
@Suppressannotations