getClient() thread safety.

[jgreen210]

Expected behavior

Thread-safe implementation.

Observed behavior

N/A.

Steps to reproduce

Call init() and notify() on different threads.

Version

5.0.0.

Additional information

All code accessing the Bugsnag class client field:

bugsnag-android/bugsnag-android-core/src/main/java/com/bugsnag/android/Bugsnag.java

Line 26 in c63c9f4

static Client client;

...should synchronize on the lock field:

bugsnag-android/bugsnag-android-core/src/main/java/com/bugsnag/android/Bugsnag.java

Line 23 in c63c9f4

private static final Object lock = new Object();

...so there is a java memory model happens-before relationship between init()'s writes and getClient()'s reads (i.e. a memory barrier):

bugsnag-android/bugsnag-android-core/src/main/java/com/bugsnag/android/Bugsnag.java

Line 366 in c63c9f4

public static Client getClient() {

Otherwise, if use multiple threads, one of the notify() methods could throw an IllegalStateException even if init() has been called.

[mattdyoung]

Hi @jgreen210

Thanks for the report. You're right that in theory this could result in an IllegalStateException if you're starting threads that call Bugsnag.notify at the same time as Bugsnag.start is called.

In practice we recommend calling Bugsnag.start in the onCreate callback of your Application subclass, which makes this failure scenario less likely, but we plan to investigate viable options to prevent this. Using a lock for every method could have a significant impact on performance so we're likely to need a different solution.

[jgreen210]

I'd prefer bugsnag to be guaranteed to operate correctly than to have the highest possible bug-recording rate. :-)

Android has quite a few threads in general - main, thread pools, AIDL, GLThread... It's worse with react native, where there's the native module thread too, which runs any android code you invoke from JavaScript. Lots of people will use Bugsnag.notifiy() from a non-main thread and expect it to always work.

If only enable bugsnag if user has given consent, and control this from js code, the Bugsnag.start() might be done via a react native native module call. Could post a Handler in the android code to do this to do it on the main thread, but many people won't do that, and it looks like bugsnag could be made to support this from any thread.

For Bugnsag.notify(), it would be possible for it to post Handlers to the main thread (assuming Bugsnag.start() does the same thing). However, that's implemented with locking too, so there wouldn't be any performance advantage.

It's not possible to reimplement getClient() asynchronously with Handlers without changing the API.

I think the correct fix is to make getClient() use locking (or an AtomicReference, if start() can be made to work with that too).

[xljones]

Hey @jgreen210, thanks for your additional comment, we will take this into consideration.

[xljones]

Hey @jgreen210, inline with your request, we've added thread-safety to the client in this SDK to ensure that the client's not used before being fully started in #1638

We've also added a new method Bugsnag.isStarted() which reports whether the Bugsnag client is started in #1640. This can be used to check whether a call to notify() is okay to make at that time.

This is all released in https://github.com/bugsnag/bugsnag-android/releases/tag/v5.22.0