Enable Android uncaught exception capture - #200
Conversation
waltjones
changed the title
cc1d94c to
5da1c01
Compare
5da1c01 to
3ec6749
Compare
waltjones
changed the title
jondeandres
left a comment
jondeandres
left a comment
There was a problem hiding this comment.
@basoko can you take a look at this?
| DiskQueue queue = new DiskQueue.Builder() | ||
| .queueFolder(folder) | ||
| .build(); | ||
| if (config.sender() != sender) { |
There was a problem hiding this comment.
I don't know details about how tihs works, but where config.sender is set and when it can be equal to sender?
There was a problem hiding this comment.
sender is the default sender created further up in this method. config.sender is the sender actually applied to the config. It may still be the default, or may have been updated by the configProvider. What this block does is, close the default sender we just created if it was removed from the config.
The != operator will compare the instance of the class, so this will detect if the new sender is a new instance of the same class and still correctly close the default.
|
|
||
| private Payload readFromFile(boolean removeFile) { | ||
| File eventFile = getFiles().get(0); | ||
| List<File> files = getFiles(); |
| @@ -16,6 +16,12 @@ repositories { | |||
|
|
|||
| apply plugin: 'com.android.library' | |||
There was a problem hiding this comment.
I think that maybe we should move the rollbar-android to it's own repository at some point, not now, as it's making more complex the build system, In the end they are quite different, its not just a java project. What do you think?
| /** | ||
| * Handle all uncaught errors on all threads with the current notifier. | ||
| */ | ||
| public void handleUncaughtErrors() { |
There was a problem hiding this comment.
Is there any reason to not make the underlying notifier set the UncaughtExceptionHandler as the default one? When the Config.handleUncaughtErrors is set? I think maybe it'd be better to haveit set as the default one instead of only to the current thread.
There was a problem hiding this comment.
I'm concerned about the change in behavior without a major version bump. For Android, it hadn't worked at all, so I'm not worried about it. For other targets, going from per thread to global will be a big change for some setups.
There was a problem hiding this comment.
What's the change in behavior a part of having an uncaught handler for every thread instead of only for the current one in which the notifier is created? I might be wrong, but looks like a fix as now we don't capture all uncaught exceptions...
There was a problem hiding this comment.
If you say change it, I'll change it. But yes, the existing interface both takes a thread object and specifies in the comment that it's per thread, so I took that to be fully intentional. https://github.com/rollbar/rollbar-java/blob/master/rollbar-java/src/main/java/com/rollbar/notifier/Rollbar.java#L80-L98
There was a problem hiding this comment.
Ok, I think we can be conservative with that change and don't do it.
There was a problem hiding this comment.
👍 Sounds good. I like that this also keeps the current PR in the scope of the Android issue.
If we separate the Android SDK, that might be a good time to plan a major version and update this.
|
@basoko Thank you! |
3 participants
Fixes: rollbar/rollbar-react-native#104
This PR fixes several issues preventing uncaught exception capture from working in Android.
Ensures the default sender uses
DiskQueueThe previous code looks like it is setting up a
DiskQueue, but the conditionalif (config.sender() == null)executes afterconfigProvider.provide(defaultConfig)causesbuild()to be called on the config. At this point,config.sender()can never be null even if the suppliedconfigProvidertries to set it to null. (Keep in mind this doesn't only affect rollbar-react-native, but all Android apps.) So the defaultConcurrentLinkedQueuewas being used. This won't work for Android uncaught exceptions, because after building the payload the sender will release the thread. The app will terminate, and the payload is lost. WithDiskQueue, the payload is stored to disk and sent on the next app start.This appears to be the pull request where this would have stopped working:
#168
Closes the default sender when a custom sender is present
#168 tried changing the initialization order so this step isn't needed, which led (in part) to the current issue.
Fixes
IndexOutOfBoundsExceptioninDiskQueue.poll()There is a bug where calling
poll()(orpeek()) tries to reference the first element on the list while the list is empty. The uncaughtIndexOutOfBoundsExceptionkeeps this queue from working. The exception will be hit anytime the queue of pending payloads is empty when the polling interval triggers.Uses
setDefaultUncaughtExceptionHandlerThe existing
setUncaughtExceptionHandleris per thread. Even if a public interface were to allow setting this for each thread, for popular frameworks like React Native that create their own threads, this would be impossible for the application to manage. This fix ensures exceptions on all threads will bubble up to the handler. The existing handler-per-thread behavior is preserved for non-android targets.Updates gradle config to run tests, lint and coverage for the android target.
Adds tests for the android target.