diff --git a/bugsnag-spring/src/main/java/com/bugsnag/BugsnagAsyncExceptionHandler.java b/bugsnag-spring/src/main/java/com/bugsnag/BugsnagAsyncExceptionHandler.java index c259b012..d0361cc0 100644 --- a/bugsnag-spring/src/main/java/com/bugsnag/BugsnagAsyncExceptionHandler.java +++ b/bugsnag-spring/src/main/java/com/bugsnag/BugsnagAsyncExceptionHandler.java @@ -24,7 +24,7 @@ public BugsnagAsyncExceptionHandler(final Bugsnag bugsnag) { @Override public void handleUncaughtException(Throwable throwable, Method method, Object... obj) { - if (bugsnag.getConfig().shouldSendUncaughtExceptions()) { + if (bugsnag.getConfig().getAutoDetectErrors()) { HandledState handledState = HandledState.newInstance( SeverityReasonType.REASON_UNHANDLED_EXCEPTION_MIDDLEWARE, Collections.singletonMap("framework", "Spring"), diff --git a/bugsnag-spring/src/main/java/com/bugsnag/BugsnagJakartaMvcExceptionHandler.java b/bugsnag-spring/src/main/java/com/bugsnag/BugsnagJakartaMvcExceptionHandler.java index ac41ca01..0198b66d 100644 --- a/bugsnag-spring/src/main/java/com/bugsnag/BugsnagJakartaMvcExceptionHandler.java +++ b/bugsnag-spring/src/main/java/com/bugsnag/BugsnagJakartaMvcExceptionHandler.java @@ -33,7 +33,7 @@ public ModelAndView resolveException(HttpServletRequest request, Object handler, java.lang.Exception ex) { - if (bugsnag.getConfig().shouldSendUncaughtExceptions()) { + if (bugsnag.getConfig().getAutoDetectErrors()) { HandledState handledState = HandledState.newInstance( SeverityReasonType.REASON_UNHANDLED_EXCEPTION_MIDDLEWARE, Collections.singletonMap("framework", "Spring"), diff --git a/bugsnag-spring/src/main/java/com/bugsnag/BugsnagScheduledTaskExceptionHandler.java b/bugsnag-spring/src/main/java/com/bugsnag/BugsnagScheduledTaskExceptionHandler.java index 1dc5d10a..3e33d088 100644 --- a/bugsnag-spring/src/main/java/com/bugsnag/BugsnagScheduledTaskExceptionHandler.java +++ b/bugsnag-spring/src/main/java/com/bugsnag/BugsnagScheduledTaskExceptionHandler.java @@ -27,7 +27,7 @@ public void handleError(Throwable throwable) { return; } - if (bugsnag.getConfig().shouldSendUncaughtExceptions()) { + if (bugsnag.getConfig().getAutoDetectErrors()) { HandledState handledState = HandledState.newInstance( SeverityReasonType.REASON_UNHANDLED_EXCEPTION_MIDDLEWARE, Collections.singletonMap("framework", "Spring"), diff --git a/bugsnag-spring/src/test/java/com/bugsnag/SpringMvcTest.java b/bugsnag-spring/src/test/java/com/bugsnag/SpringMvcTest.java index 075f42ab..25f15f55 100644 --- a/bugsnag-spring/src/test/java/com/bugsnag/SpringMvcTest.java +++ b/bugsnag-spring/src/test/java/com/bugsnag/SpringMvcTest.java @@ -71,7 +71,7 @@ public void setUp() { delivery = mock(Delivery.class); bugsnag.setDelivery(delivery); - bugsnag.getConfig().setSendUncaughtExceptions(true); + bugsnag.getConfig().setAutoDetectErrors(true); bugsnag.getConfig().setAutoCaptureSessions(true); // Cannot reset the session count on the bugsnag bean for each test, so note @@ -102,7 +102,7 @@ public void bugsnagNotifyWhenUncaughtControllerException() { @Test public void noBugsnagNotifyWhenSendUncaughtExceptionsFalse() { - bugsnag.getConfig().setSendUncaughtExceptions(false); + bugsnag.getConfig().setAutoDetectErrors(false); callRuntimeExceptionEndpoint(); diff --git a/bugsnag/src/main/java/com/bugsnag/Bugsnag.java b/bugsnag/src/main/java/com/bugsnag/Bugsnag.java index ac705502..4072b466 100644 --- a/bugsnag/src/main/java/com/bugsnag/Bugsnag.java +++ b/bugsnag/src/main/java/com/bugsnag/Bugsnag.java @@ -9,7 +9,6 @@ import org.slf4j.LoggerFactory; import java.io.Closeable; -import java.lang.Thread.UncaughtExceptionHandler; import java.net.Proxy; import java.util.Collection; import java.util.Collections; @@ -61,12 +60,7 @@ public void rejectedExecution(Runnable runnable, ThreadPoolExecutor executor) { private final SessionTracker sessionTracker; private final FeatureFlagStore featureFlagStore; - private static final ThreadLocal THREAD_METADATA = new ThreadLocal() { - @Override - public Metadata initialValue() { - return new Metadata(); - } - }; + private static final ThreadLocal THREAD_METADATA = ThreadLocal.withInitial(Metadata::new); // // Constructors @@ -97,7 +91,7 @@ public Bugsnag(String apiKey, boolean sendUncaughtExceptions) { featureFlagStore = config.copyFeatureFlagStore(); // Automatically send unhandled exceptions to Bugsnag using this Bugsnag - config.setSendUncaughtExceptions(sendUncaughtExceptions); + config.setAutoDetectErrors(sendUncaughtExceptions); if (sendUncaughtExceptions) { ExceptionHandler.enable(this); } @@ -681,13 +675,13 @@ SessionTracker getSessionTracker() { * * @return clients which catch uncaught exceptions */ - public static Set uncaughtExceptionClients() { - UncaughtExceptionHandler handler = Thread.getDefaultUncaughtExceptionHandler(); - if (handler instanceof ExceptionHandler) { - ExceptionHandler bugsnagHandler = (ExceptionHandler) handler; - return Collections.unmodifiableSet(bugsnagHandler.uncaughtExceptionClients()); + public static Iterable uncaughtExceptionClients() { + ExceptionHandler handler = ExceptionHandler.getGlobalOrNull(); + if (handler == null) { + return Collections.emptyList(); } - return Collections.emptySet(); + + return handler.uncaughtExceptionClients(); } void addOnSession(OnSession onSession) { diff --git a/bugsnag/src/main/java/com/bugsnag/Configuration.java b/bugsnag/src/main/java/com/bugsnag/Configuration.java index ecd25f71..36e49c82 100644 --- a/bugsnag/src/main/java/com/bugsnag/Configuration.java +++ b/bugsnag/src/main/java/com/bugsnag/Configuration.java @@ -47,7 +47,7 @@ public class Configuration { Set callbacks = new CopyOnWriteArraySet<>(); private volatile boolean autoCaptureSessions = true; - private volatile boolean sendUncaughtExceptions = true; + private volatile boolean autoDetectErrors = true; private final FeatureFlagStore featureFlagStore = new FeatureFlagStore(); Configuration(String apiKey) { @@ -111,12 +111,12 @@ public boolean shouldAutoCaptureSessions() { return autoCaptureSessions; } - public void setSendUncaughtExceptions(boolean sendUncaughtExceptions) { - this.sendUncaughtExceptions = sendUncaughtExceptions; + public void setAutoDetectErrors(boolean autoDetectErrors) { + this.autoDetectErrors = autoDetectErrors; } - public boolean shouldSendUncaughtExceptions() { - return sendUncaughtExceptions; + public boolean getAutoDetectErrors() { + return autoDetectErrors; } /** diff --git a/bugsnag/src/main/java/com/bugsnag/ExceptionHandler.java b/bugsnag/src/main/java/com/bugsnag/ExceptionHandler.java index c02a3ef5..3c0bf6c5 100644 --- a/bugsnag/src/main/java/com/bugsnag/ExceptionHandler.java +++ b/bugsnag/src/main/java/com/bugsnag/ExceptionHandler.java @@ -1,46 +1,56 @@ package com.bugsnag; import java.lang.Thread.UncaughtExceptionHandler; -import java.util.Set; -import java.util.WeakHashMap; +import java.lang.ref.WeakReference; +import java.util.Arrays; +import java.util.Collections; +import java.util.Objects; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.Stream; class ExceptionHandler implements UncaughtExceptionHandler { + private static volatile ExceptionHandler singletonInstance = null; + private final UncaughtExceptionHandler originalHandler; - private final WeakHashMap clientMap = new WeakHashMap(); + private final AtomicReference[]> weakClients = new AtomicReference<>(null); + private final AtomicBoolean installed = new AtomicBoolean(false); + + /** + * Returns all the Bugsnag instances associated with this ExceptionHandler. + * + * @return an immutable iterable of the Bugsnag instances associated with this ExceptionHandler (never null) + */ + Iterable uncaughtExceptionClients() { + WeakReference[] clientRefs = weakClients.get(); + if (clientRefs == null || clientRefs.length == 0) { + return Collections.emptySet(); + } - Set uncaughtExceptionClients() { - return clientMap.keySet(); + return () -> Stream.of(clientRefs) + .map(WeakReference::get) + .filter(Objects::nonNull) + .iterator(); } static void enable(Bugsnag bugsnag) { - UncaughtExceptionHandler currentHandler = Thread.getDefaultUncaughtExceptionHandler(); + ExceptionHandler handler = getGlobalOrCreate(); + handler.add(bugsnag); - // Find or create the Bugsnag ExceptionHandler - ExceptionHandler bugsnagHandler; - if (currentHandler instanceof ExceptionHandler) { - bugsnagHandler = (ExceptionHandler) currentHandler; - } else { - bugsnagHandler = new ExceptionHandler(currentHandler); - Thread.setDefaultUncaughtExceptionHandler(bugsnagHandler); + if (handler.installed.compareAndSet(false, true)) { + Thread.setDefaultUncaughtExceptionHandler(handler); } - - // Subscribe this bugsnag to uncaught exceptions - bugsnagHandler.clientMap.put(bugsnag, true); } static void disable(Bugsnag bugsnag) { - // Find the Bugsnag ExceptionHandler - UncaughtExceptionHandler currentHandler = Thread.getDefaultUncaughtExceptionHandler(); - if (currentHandler instanceof ExceptionHandler) { - // Unsubscribe this bugsnag from uncaught exceptions - ExceptionHandler bugsnagHandler = (ExceptionHandler) currentHandler; - bugsnagHandler.clientMap.remove(bugsnag); - - // Remove the Bugsnag ExceptionHandler if no clients are subscribed - if (bugsnagHandler.clientMap.size() == 0) { - Thread.setDefaultUncaughtExceptionHandler(bugsnagHandler.originalHandler); - } + ExceptionHandler handler = getGlobalOrNull(); + if (handler == null) { + return; // No handler installed, so nothing to disable + } + + if (handler.remove(bugsnag)) { + handler.cleanup(); } } @@ -51,10 +61,13 @@ static void disable(Bugsnag bugsnag) { @Override public void uncaughtException(java.lang.Thread thread, Throwable throwable) { // Notify any subscribed clients of the uncaught exception - for (Bugsnag bugsnag : clientMap.keySet()) { - if (bugsnag.getConfig().shouldSendUncaughtExceptions()) { + for (Bugsnag bugsnag : uncaughtExceptionClients()) { + if (bugsnag.getConfig().getAutoDetectErrors()) { HandledState handledState = HandledState.newInstance( - HandledState.SeverityReasonType.REASON_UNHANDLED_EXCEPTION, Severity.ERROR); + HandledState.SeverityReasonType.REASON_UNHANDLED_EXCEPTION, + Severity.ERROR + ); + bugsnag.notify(throwable, handledState, thread); } } @@ -68,4 +81,120 @@ public void uncaughtException(java.lang.Thread thread, Throwable throwable) { throwable.printStackTrace(System.err); } } + + @SuppressWarnings("unchecked") + void add(Bugsnag bugsnag) { + WeakReference newRef = new WeakReference<>(bugsnag); + + while (true) { + final WeakReference[] currentRefs = weakClients.get(); + + if (currentRefs != null) { + // Create a new array with the new client added + WeakReference[] newRefs = new WeakReference[currentRefs.length + 1]; + int index = 0; + for (WeakReference ref : currentRefs) { + // Copy existing non-garbage collected references + // It's not as fast as System.arraycopy, but it avoids needing even more copies of the array + if (ref.get() != null) { + newRefs[index++] = ref; + } + } + newRefs[index++] = newRef; + + if (index < newRefs.length) { + // If some references were garbage collected, create a smaller array + newRefs = Arrays.copyOf(newRefs, index); + } + + // Attempt to update the reference atomically + if (weakClients.compareAndSet(currentRefs, newRefs)) { + return; // Successfully added the client + } + } else { + WeakReference[] newRefs = new WeakReference[] { + newRef + }; + + // Attempt to update the reference atomically + if (weakClients.compareAndSet(null, newRefs)) { + return; // Successfully added the client + } + } + } + } + + @SuppressWarnings("unchecked") + boolean remove(Bugsnag bugsnag) { + while (true) { + final WeakReference[] currentRefs = weakClients.get(); + if (currentRefs == null || currentRefs.length == 0) { + return false; + } + + // Create a new array with the specified client removed + WeakReference[] newRefs = new WeakReference[currentRefs.length - 1]; + int index = 0; + for (WeakReference ref : currentRefs) { + Bugsnag client = ref.get(); + if (client != null && client != bugsnag) { + if (index >= newRefs.length) { + // if we reached this point, then 'bugsnag' wasn't in the list - this will always be + // the last element in the list, and it wasn't the one we are trying to remove so we + // can exit here + return false; + } + newRefs[index++] = ref; + } + } + + if (index < newRefs.length) { + // If some references were garbage collected, create a smaller array + newRefs = Arrays.copyOf(newRefs, index); + } + + if (weakClients.compareAndSet(currentRefs, newRefs)) { + // Return true if the array changed (target removed or GC'd refs cleaned) + return index < currentRefs.length; + } + } + } + + private void cleanup() { + WeakReference[] refs = weakClients.get(); + if (refs != null && refs.length > 0) { + // this instance still has clients + return; + } + + if (isCurrentInstalledHandler() && installed.compareAndSet(true, false)) { + Thread.setDefaultUncaughtExceptionHandler(originalHandler); + } + } + + /** + * Return true if this instance is currently the default uncaught exception handler. If this instance is installed + * as a handler, but is not the outermost/current handler this will return false (meaning it is not safe to + * uninstall this handler as it is possibly being delegated to by another crash handler). + * + * @return true if this instance is currently the default uncaught exception handler, false otherwise + */ + private boolean isCurrentInstalledHandler() { + return Thread.getDefaultUncaughtExceptionHandler() == this; + } + + static ExceptionHandler getGlobalOrCreate() { + if (singletonInstance == null) { + synchronized (ExceptionHandler.class) { + if (singletonInstance == null) { + singletonInstance = new ExceptionHandler(Thread.getDefaultUncaughtExceptionHandler()); + } + } + } + return singletonInstance; + } + + static ExceptionHandler getGlobalOrNull() { + return singletonInstance; + } } diff --git a/bugsnag/src/test/java/com/bugsnag/BugsnagTest.java b/bugsnag/src/test/java/com/bugsnag/BugsnagTest.java index 522064ba..7c47ab15 100644 --- a/bugsnag/src/test/java/com/bugsnag/BugsnagTest.java +++ b/bugsnag/src/test/java/com/bugsnag/BugsnagTest.java @@ -21,8 +21,8 @@ import java.net.Proxy; import java.util.HashMap; +import java.util.Iterator; import java.util.Map; -import java.util.Set; import java.util.regex.Pattern; public class BugsnagTest { @@ -69,8 +69,8 @@ public void testIgnoreClasses() { // Ignore both (compile patterns for exact matches) bugsnag.setDiscardClasses( - Pattern.compile(Pattern.quote(RuntimeException.class.getName())), - Pattern.compile(Pattern.quote(TestException.class.getName())) + Pattern.compile(Pattern.quote(RuntimeException.class.getName())), + Pattern.compile(Pattern.quote(TestException.class.getName())) ); assertFalse(bugsnag.notify(new RuntimeException())); assertFalse(bugsnag.notify(new TestException())); @@ -519,8 +519,12 @@ public void testSerialization() { @Test(expected = UnsupportedOperationException.class) public void testUncaughtHandlerModification() { - Set bugsnags = Bugsnag.uncaughtExceptionClients(); - bugsnags.clear(); + Iterator iterator = Bugsnag.uncaughtExceptionClients().iterator(); + while (iterator.hasNext()) { + //noinspection resource + iterator.next(); + iterator.remove(); // not supported + } } // Test exception class diff --git a/features/fixtures/scenarios/src/main/java/com/bugsnag/TestHooks.java b/features/fixtures/scenarios/src/main/java/com/bugsnag/TestHooks.java index 1997abce..929943ab 100644 --- a/features/fixtures/scenarios/src/main/java/com/bugsnag/TestHooks.java +++ b/features/fixtures/scenarios/src/main/java/com/bugsnag/TestHooks.java @@ -3,6 +3,6 @@ public class TestHooks { public static void disableSendUncaughtExceptions(Bugsnag bugsnag) { - bugsnag.getConfig().setSendUncaughtExceptions(false); + bugsnag.getConfig().setAutoDetectErrors(false); } }