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

Fix iOS tracer concurrency #535

Merged
merged 1 commit into from
Sep 25, 2023
Merged

Conversation

louiszawadzki
Copy link
Contributor

What does this PR do?

The testTracingConcurrently would regularly fail on the CI, so I took a look at it. I set the number of iterations to 3000 and it was failing ~50% of the times.
There were 2 main reasons for the failures.

First, a finishSpan on a different thread would be called before we got the value of lastResolvedValue in the test. It is a totally fine use case, but it would set the lastResolvedValue to nil and it would fail to unwrap the value later in the test

Then, the call to span.finish was not thread-safe and so there were cases where the span.finish would somehow fail and we finished the tests with 1 or 2 remaining spans in the spanDictionary.

I ran the test with 3000 iterations about 40 times with the proposed changes and it did not fail once.

Why we had this feature

Digging into the history and why we're using objc_sync_enter, I could trace it down to this commit (and this comment more specifically).
We were trying to make sure the Tracer wasn't accessed by 2 different thread, so indeed I think we should also wrap span.finish in the mutex.
In any case this was a valid concern for the bridge package, but I'm not so sure it's needed anymore since all calls to the RN DDTrace module now run on the same thread anyway.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)
  • If this PR is auto-generated, please make sure also to manually update the code related to the change

@louiszawadzki louiszawadzki requested a review from a team as a code owner September 25, 2023 09:59
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

The fix looks all right 👌.

In any case this was a valid concern for the bridge package, but I'm not so sure it's needed anymore since all calls to the RN DDTrace module now run on the same thread anyway.

☝️ Indeed, it does not make much sense today IMO. If I get things right, this:

- (dispatch_queue_t)methodQueue {
return [RNQueue getSharedQueue];
}

guarantees that all DdTraceImplementation will run on a single queue (think: single thread). The requirement we assert in tests: "DdTraceImplementation is thread safe" comes from previous setup with multiple queues. We may not need it anymore: neither the concurrency test nor objc_sync_enter | exit locks.

@louiszawadzki
Copy link
Contributor Author

I'd say let's keep the lock until we can properly cover the methodQueue snippet by tests.

@louiszawadzki louiszawadzki merged commit d7bd922 into develop Sep 25, 2023
3 checks passed
@louiszawadzki louiszawadzki deleted the louiszawadzki/fix-ios-flaky-test branch September 25, 2023 11:10
louiszawadzki added a commit that referenced this pull request Oct 30, 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.

2 participants