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

Breadcrumb API is not thread-safe #616

Closed
dtorres opened this issue May 22, 2020 · 7 comments · Fixed by #743
Closed

Breadcrumb API is not thread-safe #616

dtorres opened this issue May 22, 2020 · 7 comments · Fixed by #743
Labels
bug Confirmed bug released This feature/bug fix has been released

Comments

@dtorres
Copy link

dtorres commented May 22, 2020

Description

While running our app under the thread sanitiser, it frequently hits a race condition on the Breadcrumb API as seen here:

Screenshot 2020-05-22 at 11 09 20

Screenshot 2020-05-22 at 11 09 41

Issue

Environment

Library versions:

  • Bugsnag version (from your Podfile, Podfile.lock, or elsewhere):
  • CocoaPods version (if any) (pod -v):
  • Carthage version (if any):
  • iOS/tvOS/macOS version(s)
  • debug mode or production?:
  • How are you initializing Bugsnag? Can you share a snippet of your
    AppDelegate where Bugsnag is configured?

Example code snippet

import Bugsnag

// (Insert code sample to reproduce the problem)
Error messages:

@mattdyoung
Copy link

Thanks for the report @dtorres. I can reproduce this so we'll take a look and try to determine the cause.

Have you seen any evidence of this causing crashes or other issues or has it just been flagged by thread sanitizer so far?

Until we can identify a fix, you may be able to work around this by using your own FIFO queue (e.g. NSOperationQueue with a maxConcurrentOperationCount count of 1) for calling leaveBreadcrumb().

@mattdyoung mattdyoung added backlog We hope to fix this feature/bug in the future bug Confirmed bug labels May 22, 2020
@dtorres
Copy link
Author

dtorres commented May 22, 2020

Thanks for the report @dtorres. I can reproduce this so we'll take a look and try to determine the cause.

Have you seen any evidence of this causing crashes or other issues or has it just been flagged by thread sanitizer so far?

We haven't seen this particular stack trace as a crash yet but a memory corruption issue (such as this) can cause other pieces of code to crash if they are unlucky enough to step on that same memory address later.

This race condition is now more likely to happen in our app as we have backgrounded most calls to the breadcrumb API as it isn't suited for main thread use (See #563).

Until we can identify a fix, you may be able to work around this by using your own FIFO queue (e.g. NSOperationQueue with a maxConcurrentOperationCount count of 1) for calling leaveBreadcrumb().

We are wrapping the calls inside of a lock to work around this.

@mattdyoung
Copy link

Understood. We'll need to investigate further on our side to determine the best way to fix. I'll update this issue when we know more.

@iwasrobbed-ks
Copy link

I saw this as well today and it's basically because you're writing a new breadcrumb within readWriteQueue but then not protecting reads for breadcrumbs within the same readWriteQueue. That violates the read/write approach that you all took since reads need equal protection

Screen Shot 2020-06-05 at 10 56 34 AM

Screen Shot 2020-06-05 at 10 57 00 AM

@johnkiely1
Copy link
Member

@rob-keepsafe

Thanks for this, we will review and update this issue as necessary.

@RuiAAPeres
Copy link

Any luck on this?

@bugsnagbot bugsnagbot added scheduled Work is starting on this feature/bug and removed backlog We hope to fix this feature/bug in the future labels Jul 3, 2020
@tomlongridge tomlongridge linked a pull request Jul 7, 2020 that will close this issue
@johnkiely1
Copy link
Member

This has now been fixed in V6.1.1

@johnkiely1 johnkiely1 added released This feature/bug fix has been released and removed scheduled Work is starting on this feature/bug labels Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug released This feature/bug fix has been released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants