-
Notifications
You must be signed in to change notification settings - Fork 35
Background thread support #67
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
Background thread support #67
Conversation
| /// </summary> | ||
| private readonly Dictionary<string, string> _clientAttributes = new Dictionary<string, string>(); | ||
|
|
||
| internal readonly Stack<BacktraceReport> BackgroundExceptions = new Stack<BacktraceReport>(); |
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.
Why is this Stack instead of Queue?
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.
Hmm yes, you're right. Queue makes more sense here
| // and let update method send data to Backtrace. | ||
| if (Thread.CurrentThread.ManagedThreadId != _current.ManagedThreadId) | ||
| { | ||
| var report = new BacktraceReport(exception, attributes, attachmentPaths); |
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.
This seems to be repeated 3 times. Can we pull this into a function?
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.
Not really because of different parameters that this function require and cost of BacktraceReport creation
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.
Okay I see. Since this gets called in Update method we need to consider cost of BacktraceReport creation.
| /// Update native client internal ANR timer. | ||
| /// </summary> | ||
| private void Update() | ||
| private void LateUpdate() |
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.
Why is this LateUpdate now? What happens in Update?
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.
To give main thread to update before an anr thread
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.
Ah okay that makes sense
rqbacktrace
left a comment
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.
Looks good to me! Just had a few questions
vlussenburg
left a comment
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.
didn't fully understand the issue but seeing the test made it clear! 👌
…se-positive anrs (#69) * Background thread support (#67) * Background thread support * Fixed exception flow * Removed renamed file * Detect false-positive ANRs report and prevent ANR watchdog from sending them (#66) * Detect false-positive ANRs report and prevent ANR watchdog from sending them + anr thread improvements * Fixed typos + better documentation - added Samy's advices * Attachment support (#68) * Attachment support on Android - iOS shouldn't compile * Updated the latest version of backtrace-android libraries and also adjust iOS integration code - code should now compile on iOS * Native attachment support * Attachment improvements * Updated label * Native client updates * Android libraries * Version update * 3.4.0-rc1 * Prevents from reading empty attachments * Arabic language support (#70) * Fixed invalid date in log manager when calendar is unsupported * Use single method to generate UTC timestamps + move it from extension code to static class to avoid unnecessary allocation * Fixed line endings * Updated native library * Removed debug logs from native library * Updated library version * Added invariant culture to rest of the code * native library update * Fixed issue in the readme file * Correct attachment name (#73) * Correct attachment name * Moved constants to const variables * Prepare for final release * Fixed typo in function name * Changelog change
Why
Our users would like to start using backtraceClient instance from background threads/coroutines/async methods. In examples that we saw in the past they're sharing an instance of backtraceClient between threads - which is kinda not safe, and use it to send reports when an exception occurs.
Our users would like to use Backtrace to detect and capture exceptions that background threads generate.
With this approach, when they will use backtraceClient to send a report from any background thread, because of Unity engine dependency we won't be able to use Unity API to generate the report - for example, we won't be able to start coroutine or generate attributes.
This diff adds background exception cache with the respect of exception information (so everything related to the background thread will be still available). Once Update method fire in the Backtrace client we will be able to send it from main thread.
Testing strategy
Send report/exception/message from Backtrace client from background thread
Capture unhandled thread exception generated by background thread in Backtrace
Example code that previously didn't work
or