diff --git a/CHANGELOG.md b/CHANGELOG.md index e7c4fd9fc7..85dc62b555 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,8 @@ - This was causing Sentry SDK to log warnings: "Sentry Log is disabled and this 'logger' call is a no-op." - Do not use Sentry logging API in Log4j2 if logs are disabled ([#4573](https://github.com/getsentry/sentry-java/pull/4573)) - This was causing Sentry SDK to log warnings: "Sentry Log is disabled and this 'logger' call is a no-op." +- SDKs send queue is no longer shutdown immediately on re-init ([#4564](https://github.com/getsentry/sentry-java/pull/4564)) + - This means we're no longer losing events that have been enqueued right before SDK re-init. - Reduce scope forking when using OpenTelemetry ([#4565](https://github.com/getsentry/sentry-java/pull/4565)) - `Sentry.withScope` now has the correct current scope passed to the callback. Previously our OpenTelemetry integration forked scopes an additional. - Overall the SDK is now forking scopes a bit less often. diff --git a/sentry/src/main/java/io/sentry/ShutdownHookIntegration.java b/sentry/src/main/java/io/sentry/ShutdownHookIntegration.java index 3d5bccc3d7..9a70e31cee 100644 --- a/sentry/src/main/java/io/sentry/ShutdownHookIntegration.java +++ b/sentry/src/main/java/io/sentry/ShutdownHookIntegration.java @@ -32,7 +32,8 @@ public void register(final @NotNull IScopes scopes, final @NotNull SentryOptions Objects.requireNonNull(options, "SentryOptions is required"); if (options.isEnableShutdownHook()) { - thread = new Thread(() -> scopes.flush(options.getFlushTimeoutMillis())); + thread = + new Thread(() -> scopes.flush(options.getFlushTimeoutMillis()), "sentry-shutdownhook"); handleShutdownInProgress( () -> { runtime.addShutdownHook(thread); diff --git a/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java b/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java index 24f954c0c1..149c04f2f8 100644 --- a/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java +++ b/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java @@ -174,20 +174,23 @@ public void close(final boolean isRestarting) throws IOException { executor.shutdown(); options.getLogger().log(SentryLevel.DEBUG, "Shutting down"); try { - // We need a small timeout to be able to save to disk any rejected envelope - long timeout = isRestarting ? 0 : options.getFlushTimeoutMillis(); - if (!executor.awaitTermination(timeout, TimeUnit.MILLISECONDS)) { - options - .getLogger() - .log( - SentryLevel.WARNING, - "Failed to shutdown the async connection async sender within " - + timeout - + " ms. Trying to force it now."); - executor.shutdownNow(); - if (currentRunnable != null) { - // We store to disk any envelope that is currently being sent - executor.getRejectedExecutionHandler().rejectedExecution(currentRunnable, executor); + // only stop sending events on a real shutdown, not on a restart + if (!isRestarting) { + // We need a small timeout to be able to save to disk any rejected envelope + long timeout = options.getFlushTimeoutMillis(); + if (!executor.awaitTermination(timeout, TimeUnit.MILLISECONDS)) { + options + .getLogger() + .log( + SentryLevel.WARNING, + "Failed to shutdown the async connection async sender within " + + timeout + + " ms. Trying to force it now."); + executor.shutdownNow(); + if (currentRunnable != null) { + // We store to disk any envelope that is currently being sent + executor.getRejectedExecutionHandler().rejectedExecution(currentRunnable, executor); + } } } } catch (InterruptedException e) { diff --git a/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt b/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt index c7ed2b46aa..c3a7f2ce04 100644 --- a/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt +++ b/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt @@ -352,16 +352,22 @@ class AsyncHttpTransportTest { } @Test - fun `close with isRestarting true does not await termination`() { - fixture.sentryOptions.flushTimeoutMillis = 123 + fun `close shuts down the executor and runs executing runnable through rejectedExecutionHandler`() { + val rejectedExecutionHandler = mock() val sut = fixture.getSUT() - sut.close(true) + val runnable = mock() - verify(fixture.executor).awaitTermination(eq(0), eq(TimeUnit.MILLISECONDS)) + // Emulate a runnable currently being executed + sut.injectForField("currentRunnable", runnable) + whenever(fixture.executor.rejectedExecutionHandler).thenReturn(rejectedExecutionHandler) + sut.close(false) + + verify(fixture.executor).shutdownNow() + verify(rejectedExecutionHandler).rejectedExecution(eq(runnable), eq(fixture.executor)) } @Test - fun `close shuts down the executor and runs executing runnable through rejectedExecutionHandler`() { + fun `does not shut down executor immediately on restart`() { val rejectedExecutionHandler = mock() val sut = fixture.getSUT() val runnable = mock() @@ -371,8 +377,9 @@ class AsyncHttpTransportTest { whenever(fixture.executor.rejectedExecutionHandler).thenReturn(rejectedExecutionHandler) sut.close(true) - verify(fixture.executor).shutdownNow() - verify(rejectedExecutionHandler).rejectedExecution(eq(runnable), eq(fixture.executor)) + verify(fixture.executor).shutdown() + verify(fixture.executor, never()).shutdownNow() + verify(rejectedExecutionHandler, never()).rejectedExecution(eq(runnable), eq(fixture.executor)) } @Test