-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RUMM-2202 Ensure thread safety #936
Conversation
f69c36a
to
5c4a506
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I've left a suggestion in the test and a question regarding the TODO item, but overall it is non-blocking.
import androidx.annotation.UiThread | ||
|
||
class Foo { | ||
@UiThread |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import androidx.annotation.UiThread | |
class Foo { | |
@UiThread | |
import androidx.annotation.WorkerThread | |
class Foo { | |
@WorkerThread |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oups
@@ -6,6 +6,9 @@ | |||
|
|||
package com.datadog.android.v2.core.internal.storage | |||
|
|||
internal fun interface BatchConfirmation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we removing functional interface property here (and in the other interfaces), because it is complicated to get annotation in case of lamba implementation of the functional interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly
rootScope.handleEvent(event, writer) | ||
} else if (event is RumRawEvent.SendTelemetry) { | ||
@Suppress("ThreadSafety") // TODO RUMM-1503 delegate to another thread |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to do it here? I mean generally speaking yes (because we known only the interface of the writer
, but not implementation), but if we are talking about this particular case then normally writer
is a ScheduledWriter
, so when writer.write
is called from the TelemetryEventHandler
, write operation will be performed on the worker thread (because ScheduledWriter
bundles ExecutorService
).
So in the end we delegate this call to the worker thread for nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing here is to provide a way to enforce the thread safety, and maybe we can find another way to make sure the write operation is called in the right thread. Maybe the scheduled writer needs to use a different interface with an @AnyThread
annotation? Anyway this will be handled in the TODO task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
5c4a506
to
5cc14b3
Compare
What does this PR do?
@WorkerThread
/@UiThread
…