Skip to content
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

Merged
merged 2 commits into from
May 24, 2022

Conversation

xgouchet
Copy link
Member

What does this PR do?

  • adds a Detekt rule to verify the call compatibility for methods annotated with @WorkerThread/ @UiThread
  • Start flagging internal methods with the relevant annotations

@xgouchet xgouchet requested a review from a team as a code owner May 23, 2022 12:23
@xgouchet xgouchet changed the base branch from develop to feature/sdkv2 May 23, 2022 12:23
@xgouchet xgouchet force-pushed the xgouchet/RUMM-2202/thread_safety branch from f69c36a to 5c4a506 Compare May 23, 2022 13:35
Copy link
Member

@0xnm 0xnm left a 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.

Comment on lines +306 to +309
import androidx.annotation.UiThread

class Foo {
@UiThread
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import androidx.annotation.UiThread
class Foo {
@UiThread
import androidx.annotation.WorkerThread
class Foo {
@WorkerThread

Copy link
Member Author

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 {
Copy link
Member

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?

Copy link
Member Author

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
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

@mariusc83 mariusc83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@xgouchet xgouchet force-pushed the xgouchet/RUMM-2202/thread_safety branch from 5c4a506 to 5cc14b3 Compare May 24, 2022 08:53
@xgouchet xgouchet merged commit 7f7cbc3 into feature/sdkv2 May 24, 2022
@xgouchet xgouchet deleted the xgouchet/RUMM-2202/thread_safety branch May 24, 2022 09:51
@xgouchet xgouchet added this to the 1.16.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants