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

Data races in custom instrumentation (SentrySpan/SentryTracer) and Sentry initialization #3069

Closed
2 of 4 tasks
everuribe opened this issue May 27, 2023 · 4 comments
Closed
2 of 4 tasks

Comments

@everuribe
Copy link

everuribe commented May 27, 2023

Platform

iOS

Installed

Swift Package Manager

Version

8.5.0, 8.7.3

Steps to Reproduce

There are several observed data races in our app are occurring within Sentry processes.

One obvious instance happens when the app is initialized and [SentryANRTracker detectANRs] is called. This happens consistently even when disabling app hang reports so it appears like something that Sentry should fix. Similarly [SentryId empty] call often shows up as a data race.

image

The remainder of the data races observed are seen in the SentrySpan and SentryTracer API of the framework, specifically in the finishing process. There is some overlap of calls trying to access properties of that process (ie isFinished, wasFinished, status and possibly more).

image

We use custom instrumentation where we set up a parent SentrySpan for a process that branches out to sub-steps in other modules / threads / contexts for which we establish child spans. We call finish() once the span's works is completed, for each span. Eliminating the parent span seems to remove the occurrences of these data races.

Tested hypotheses:

  1. SentrySpan 's finishing process should only be externally called once.
    • Added a boolean stoppage that checks whether finish() had already been called and if so, prevents another call to SentrySpan.finish(). This did not seem to mitigate the frequency of data races.
  2. Sentry API doesn't handle simultaneous spans of the same flow very well. Maybe it executes some auto-finish process for the previous span of the same flow?
    • Set up a serial queue in to only process one parent span flow at a time. Added a fatalError to also make sure finish() was only called once per span. This also did not seem to have an impact.
  3. We are some times attaching child spans to a parent span after the parent no longer exists (via @TaskLocal). This did seem to be the case as some times the framework returns SentryNoOpSpan when attempting to attach the child. Maybe this causes a weird chain of calls to the finishing process of those spans as the API tries to clean up.
    • Prevented this from happening by returning early from a trace upon detecting a SentryNoOpSpan (checked for empty traceID). This also did not have an impact.

Expected Result

  • No data races when initializing app and configuring Sentry with or without app hang detection.
  • No data races in SentrySpan / SentryTracer (unless the API is not supposed to be thread safe?)

Actual Result

Our conclusion is that somehow SentrySpan and SentryTracer are not thread safe due to some internal mechanism that calls the finish process (maybe related to #3060). We see there is info about not supporting thread safety in the Native framework docs of Sentry but nothing in the Apple docs.

Data Races

Preview Give feedback
@everuribe everuribe changed the title Data races in custom instrumentation (SentrySpan/SentryTracer) and app hanging monitoring Data races in custom instrumentation (SentrySpan/SentryTracer) and Sentry initialization May 27, 2023
@philipphofmann
Copy link
Member

philipphofmann commented May 30, 2023

Thanks for reporting this, @everuribe. I fixed one data race for SentryId.empty with #3072.

We will look at the other data races and open PRs for them if needed.

@kahest kahest moved this from Needs Discussion to Backlog in Mobile & Cross Platform SDK May 31, 2023
@github-actions
Copy link

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@armcknight
Copy link
Member

This should be fixed in #3303, I'm trying to get it merged today for a new release.

@philipphofmann
Copy link
Member

This was fixed with #3303. Please reopen if you still experience the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

6 participants