Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
### Performance

- Reduce the number of SDK threads: `LifecycleWatcher` now schedules the session-end task on the shared timer executor instead of creating a dedicated `java.util.Timer` thread ([#5819](https://github.com/getsentry/sentry-java/pull/5819))
- Avoid waiting up to `shutdownTimeoutMillis` when closing the SDK with a pending transaction timeout or session-end task ([#5851](https://github.com/getsentry/sentry-java/pull/5851))
- Speed up deserialization of arbitrary JSON objects by typing numbers without throwing exceptions ([#5783](https://github.com/getsentry/sentry-java/pull/5783))

### Dependencies
Expand Down
4 changes: 4 additions & 0 deletions sentry/src/main/java/io/sentry/SentryExecutorService.java
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ public SentryExecutorService(final @Nullable SentryOptions options) {
executorService.setRemoveOnCancelPolicy(removeOnCancelPolicy);
executorService.setKeepAliveTime(keepAliveTime, keepAliveTimeUnit);
executorService.allowCoreThreadTimeOut(true);
// by default shutdown() keeps queued delayed tasks, so awaitTermination blocks for the full
// shutdown timeout whenever a long timeout is still pending. Those tasks are discarded by the
// subsequent shutdownNow() anyway, so dropping them upfront only saves the wait.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

executorService.setExecuteExistingDelayedTasksAfterShutdownPolicy(false);
}

public SentryExecutorService() {
Expand Down
22 changes: 22 additions & 0 deletions sentry/src/test/java/io/sentry/SentryExecutorServiceTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import java.util.concurrent.LinkedBlockingQueue
import java.util.concurrent.ScheduledThreadPoolExecutor
import java.util.concurrent.TimeUnit
import java.util.concurrent.atomic.AtomicBoolean
import kotlin.system.measureTimeMillis
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertFailsWith
Expand Down Expand Up @@ -110,6 +111,27 @@ class SentryExecutorServiceTest {
sentryExecutor.close(15000)
}

@Test
fun `SentryExecutorService discards pending delayed tasks on shutdown when requested`() {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we really need these tests.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the inline comment is enough to prevent regressions, so my vote would be to remove...

val sentryExecutor = SentryExecutorService(null, true, 30, TimeUnit.SECONDS)
val executor = sentryExecutor.getProperty<ScheduledThreadPoolExecutor>("executorService")
assertFalse(executor.executeExistingDelayedTasksAfterShutdownPolicy)
sentryExecutor.close(15000)
}

@Test
fun `SentryExecutorService close does not wait for a pending delayed task`() {
val sentryExecutor = SentryExecutorService(null, true, 30, TimeUnit.SECONDS)
val ran = AtomicBoolean(false)
sentryExecutor.schedule({ ran.set(true) }, 30000)

val elapsed = measureTimeMillis { sentryExecutor.close(5000) }

assertTrue(elapsed < 5000, "close blocked for ${elapsed}ms waiting on the pending task")
assertTrue(sentryExecutor.isClosed)
assertFalse(ran.get())
}

@Test
fun `SentryExecutorService isClosed returns true if executor is shutdown`() {
val executor = mock<ScheduledThreadPoolExecutor>()
Expand Down
Loading