-
-
Notifications
You must be signed in to change notification settings - Fork 341
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
Fixed deadlock during offline start. #3959
Conversation
if (nil == currentHub) { | ||
currentHub = [[SentryHub alloc] initWithClient:nil andScope:nil]; | ||
} |
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.
h: We cant remove this right now. It will break a lot of tests, we need to find another solution.
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.
Could you explain motivation behind this?
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.
If it'll try to send envelopes to that hub, they'll be just dropped. And transport should wait for a hub with a client to send something.
May be we could check scope, before enrich it.
if (scope)
[SentryDependencyContainer.sharedInstance.crashWrapper enrichScope:scope];
But that's strange solution and it could happen again in different part of code. Sentry should try to send something only when SentrySDK start finishes. With changes from this PR it's guaranteed.
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.
Could you explain motivation behind this?
This was probably put in place to make writing tests easier.
As you can see, all unit tests are broken right now.
But I agree that we need to check if the SDK is initialized before starting to use the hub.
I've checked 8.21, when it worked without issues. It creates SentryHub without enriching scope:
And there is no systemInfo call and no deadlock. |
📜 Description
See details in #3956
💡 Motivation and Context
Sentry deadlocked during offline startup.
💚 How did you test it?
I've added sleep into the crash wrapper. And when background thread tried to send envelope it locked SentrySdk @synchronized(self).
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.🔮 Next steps