From aa91b8fc8d13c7605736e6d40c251059547bc532 Mon Sep 17 00:00:00 2001 From: Lukas Bloder Date: Mon, 26 Feb 2024 15:19:20 +0100 Subject: [PATCH] Fix hub restore in async wrappers (#3225) * fix hub restoration point in wrappers: SentryWrapper, SentryTaskDecorator and SentryScheduleHook * add changelog * fix SentryScheduleHook in jakarta, api dump * Update CHANGELOG.md Co-authored-by: Alexander Dinauer * code review --------- Co-authored-by: Alexander Dinauer --- CHANGELOG.md | 2 + .../spring/jakarta/SentryTaskDecorator.java | 3 +- .../jakarta/webflux/SentryScheduleHook.java | 3 +- .../spring/jakarta/SentryTaskDecoratorTest.kt | 59 ++++++++++++++ .../jakarta/webflux/SentryScheduleHookTest.kt | 60 ++++++++++++++ .../io/sentry/spring/SentryTaskDecorator.java | 3 +- .../spring/webflux/SentryScheduleHook.java | 3 +- .../sentry/spring/SentryTaskDecoratorTest.kt | 59 ++++++++++++++ .../spring/webflux/SentryScheduleHookTest.kt | 60 ++++++++++++++ .../main/java/io/sentry/SentryWrapper.java | 9 ++- .../test/java/io/sentry/SentryWrapperTest.kt | 79 ++++++++++++++++++- 11 files changed, 330 insertions(+), 10 deletions(-) create mode 100644 sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/SentryTaskDecoratorTest.kt create mode 100644 sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/webflux/SentryScheduleHookTest.kt create mode 100644 sentry-spring/src/test/kotlin/io/sentry/spring/SentryTaskDecoratorTest.kt create mode 100644 sentry-spring/src/test/kotlin/io/sentry/spring/webflux/SentryScheduleHookTest.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index f6602434ac..09417cf844 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ - Ensure performance measurement collection is not taken too frequently ([#3221](https://github.com/getsentry/sentry-java/pull/3221)) - Fix old profiles deletion on SDK init ([#3216](https://github.com/getsentry/sentry-java/pull/3216)) +- Fix hub restore point in wrappers: SentryWrapper, SentryTaskDecorator and SentryScheduleHook ([#3225](https://github.com/getsentry/sentry-java/pull/3225)) + - We now reset the hub to its previous value on the thread where the `Runnable`/`Callable`/`Supplier` is executed instead of setting it to the hub that was used on the thread where the `Runnable`/`Callable`/`Supplier` was created. ## 7.4.0 diff --git a/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/SentryTaskDecorator.java b/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/SentryTaskDecorator.java index 7e23935c9b..5c4f37e521 100644 --- a/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/SentryTaskDecorator.java +++ b/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/SentryTaskDecorator.java @@ -15,9 +15,10 @@ public final class SentryTaskDecorator implements TaskDecorator { @Override public @NotNull Runnable decorate(final @NotNull Runnable runnable) { - final IHub oldState = Sentry.getCurrentHub(); final IHub newHub = Sentry.getCurrentHub().clone(); + return () -> { + final IHub oldState = Sentry.getCurrentHub(); Sentry.setCurrentHub(newHub); try { runnable.run(); diff --git a/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/SentryScheduleHook.java b/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/SentryScheduleHook.java index 83239ea018..2bf27f246d 100644 --- a/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/SentryScheduleHook.java +++ b/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/SentryScheduleHook.java @@ -14,9 +14,10 @@ public final class SentryScheduleHook implements Function { @Override public Runnable apply(final @NotNull Runnable runnable) { - final IHub oldState = Sentry.getCurrentHub(); final IHub newHub = Sentry.getCurrentHub().clone(); + return () -> { + final IHub oldState = Sentry.getCurrentHub(); Sentry.setCurrentHub(newHub); try { runnable.run(); diff --git a/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/SentryTaskDecoratorTest.kt b/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/SentryTaskDecoratorTest.kt new file mode 100644 index 0000000000..aa809259d1 --- /dev/null +++ b/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/SentryTaskDecoratorTest.kt @@ -0,0 +1,59 @@ +package io.sentry.spring.jakarta + +import io.sentry.Sentry +import java.util.concurrent.ExecutorService +import java.util.concurrent.Executors +import kotlin.test.AfterTest +import kotlin.test.BeforeTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNotEquals + +class SentryTaskDecoratorTest { + private val dsn = "http://key@localhost/proj" + private lateinit var executor: ExecutorService + + @BeforeTest + fun beforeTest() { + executor = Executors.newSingleThreadExecutor() + } + + @AfterTest + fun afterTest() { + Sentry.close() + executor.shutdown() + } + + @Test + fun `hub is reset to its state within the thread after decoration is done`() { + Sentry.init { + it.dsn = dsn + } + + val sut = SentryTaskDecorator() + + val mainHub = Sentry.getCurrentHub() + val threadedHub = Sentry.getCurrentHub().clone() + + executor.submit { + Sentry.setCurrentHub(threadedHub) + }.get() + + assertEquals(mainHub, Sentry.getCurrentHub()) + + val callableFuture = + executor.submit( + sut.decorate { + assertNotEquals(mainHub, Sentry.getCurrentHub()) + assertNotEquals(threadedHub, Sentry.getCurrentHub()) + } + ) + + callableFuture.get() + + executor.submit { + assertNotEquals(mainHub, Sentry.getCurrentHub()) + assertEquals(threadedHub, Sentry.getCurrentHub()) + }.get() + } +} diff --git a/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/webflux/SentryScheduleHookTest.kt b/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/webflux/SentryScheduleHookTest.kt new file mode 100644 index 0000000000..1eb06d0afe --- /dev/null +++ b/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/webflux/SentryScheduleHookTest.kt @@ -0,0 +1,60 @@ +package io.sentry.spring.jakarta.webflux + +import io.sentry.Sentry +import java.util.concurrent.ExecutorService +import java.util.concurrent.Executors +import kotlin.test.AfterTest +import kotlin.test.BeforeTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNotEquals + +class SentryScheduleHookTest { + + private val dsn = "http://key@localhost/proj" + private lateinit var executor: ExecutorService + + @BeforeTest + fun beforeTest() { + executor = Executors.newSingleThreadExecutor() + } + + @AfterTest + fun afterTest() { + Sentry.close() + executor.shutdown() + } + + @Test + fun `hub is reset to its state within the thread after hook is done`() { + Sentry.init { + it.dsn = dsn + } + + val sut = SentryScheduleHook() + + val mainHub = Sentry.getCurrentHub() + val threadedHub = Sentry.getCurrentHub().clone() + + executor.submit { + Sentry.setCurrentHub(threadedHub) + }.get() + + assertEquals(mainHub, Sentry.getCurrentHub()) + + val callableFuture = + executor.submit( + sut.apply { + assertNotEquals(mainHub, Sentry.getCurrentHub()) + assertNotEquals(threadedHub, Sentry.getCurrentHub()) + } + ) + + callableFuture.get() + + executor.submit { + assertNotEquals(mainHub, Sentry.getCurrentHub()) + assertEquals(threadedHub, Sentry.getCurrentHub()) + }.get() + } +} diff --git a/sentry-spring/src/main/java/io/sentry/spring/SentryTaskDecorator.java b/sentry-spring/src/main/java/io/sentry/spring/SentryTaskDecorator.java index f5a484536a..cb4a700696 100644 --- a/sentry-spring/src/main/java/io/sentry/spring/SentryTaskDecorator.java +++ b/sentry-spring/src/main/java/io/sentry/spring/SentryTaskDecorator.java @@ -15,9 +15,10 @@ public final class SentryTaskDecorator implements TaskDecorator { @Override public @NotNull Runnable decorate(final @NotNull Runnable runnable) { - final IHub oldState = Sentry.getCurrentHub(); final IHub newHub = Sentry.getCurrentHub().clone(); + return () -> { + final IHub oldState = Sentry.getCurrentHub(); Sentry.setCurrentHub(newHub); try { runnable.run(); diff --git a/sentry-spring/src/main/java/io/sentry/spring/webflux/SentryScheduleHook.java b/sentry-spring/src/main/java/io/sentry/spring/webflux/SentryScheduleHook.java index efa01e377d..7775d4e482 100644 --- a/sentry-spring/src/main/java/io/sentry/spring/webflux/SentryScheduleHook.java +++ b/sentry-spring/src/main/java/io/sentry/spring/webflux/SentryScheduleHook.java @@ -14,9 +14,10 @@ public final class SentryScheduleHook implements Function { @Override public Runnable apply(final @NotNull Runnable runnable) { - final IHub oldState = Sentry.getCurrentHub(); final IHub newHub = Sentry.getCurrentHub().clone(); + return () -> { + final IHub oldState = Sentry.getCurrentHub(); Sentry.setCurrentHub(newHub); try { runnable.run(); diff --git a/sentry-spring/src/test/kotlin/io/sentry/spring/SentryTaskDecoratorTest.kt b/sentry-spring/src/test/kotlin/io/sentry/spring/SentryTaskDecoratorTest.kt new file mode 100644 index 0000000000..e202deca86 --- /dev/null +++ b/sentry-spring/src/test/kotlin/io/sentry/spring/SentryTaskDecoratorTest.kt @@ -0,0 +1,59 @@ +package io.sentry.spring + +import io.sentry.Sentry +import java.util.concurrent.ExecutorService +import java.util.concurrent.Executors +import kotlin.test.AfterTest +import kotlin.test.BeforeTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNotEquals + +class SentryTaskDecoratorTest { + private val dsn = "http://key@localhost/proj" + private lateinit var executor: ExecutorService + + @BeforeTest + fun beforeTest() { + executor = Executors.newSingleThreadExecutor() + } + + @AfterTest + fun afterTest() { + Sentry.close() + executor.shutdown() + } + + @Test + fun `hub is reset to its state within the thread after decoration is done`() { + Sentry.init { + it.dsn = dsn + } + + val sut = SentryTaskDecorator() + + val mainHub = Sentry.getCurrentHub() + val threadedHub = Sentry.getCurrentHub().clone() + + executor.submit { + Sentry.setCurrentHub(threadedHub) + }.get() + + assertEquals(mainHub, Sentry.getCurrentHub()) + + val callableFuture = + executor.submit( + sut.decorate { + assertNotEquals(mainHub, Sentry.getCurrentHub()) + assertNotEquals(threadedHub, Sentry.getCurrentHub()) + } + ) + + callableFuture.get() + + executor.submit { + assertNotEquals(mainHub, Sentry.getCurrentHub()) + assertEquals(threadedHub, Sentry.getCurrentHub()) + }.get() + } +} diff --git a/sentry-spring/src/test/kotlin/io/sentry/spring/webflux/SentryScheduleHookTest.kt b/sentry-spring/src/test/kotlin/io/sentry/spring/webflux/SentryScheduleHookTest.kt new file mode 100644 index 0000000000..b09011d544 --- /dev/null +++ b/sentry-spring/src/test/kotlin/io/sentry/spring/webflux/SentryScheduleHookTest.kt @@ -0,0 +1,60 @@ +package io.sentry.spring.webflux + +import io.sentry.Sentry +import java.util.concurrent.ExecutorService +import java.util.concurrent.Executors +import kotlin.test.AfterTest +import kotlin.test.BeforeTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNotEquals + +class SentryScheduleHookTest { + + private val dsn = "http://key@localhost/proj" + private lateinit var executor: ExecutorService + + @BeforeTest + fun beforeTest() { + executor = Executors.newSingleThreadExecutor() + } + + @AfterTest + fun afterTest() { + Sentry.close() + executor.shutdown() + } + + @Test + fun `hub is reset to its state within the thread after hook is done`() { + Sentry.init { + it.dsn = dsn + } + + val sut = SentryScheduleHook() + + val mainHub = Sentry.getCurrentHub() + val threadedHub = Sentry.getCurrentHub().clone() + + executor.submit { + Sentry.setCurrentHub(threadedHub) + }.get() + + assertEquals(mainHub, Sentry.getCurrentHub()) + + val callableFuture = + executor.submit( + sut.apply { + assertNotEquals(mainHub, Sentry.getCurrentHub()) + assertNotEquals(threadedHub, Sentry.getCurrentHub()) + } + ) + + callableFuture.get() + + executor.submit { + assertNotEquals(mainHub, Sentry.getCurrentHub()) + assertEquals(threadedHub, Sentry.getCurrentHub()) + }.get() + } +} diff --git a/sentry/src/main/java/io/sentry/SentryWrapper.java b/sentry/src/main/java/io/sentry/SentryWrapper.java index 609d58be2e..1a39adee99 100644 --- a/sentry/src/main/java/io/sentry/SentryWrapper.java +++ b/sentry/src/main/java/io/sentry/SentryWrapper.java @@ -28,10 +28,10 @@ public final class SentryWrapper { * @param - the result type of the {@link Callable} */ public static Callable wrapCallable(final @NotNull Callable callable) { - final IHub oldState = Sentry.getCurrentHub(); - final IHub newHub = oldState.clone(); + final IHub newHub = Sentry.getCurrentHub().clone(); return () -> { + final IHub oldState = Sentry.getCurrentHub(); Sentry.setCurrentHub(newHub); try { return callable.call(); @@ -52,10 +52,11 @@ public static Callable wrapCallable(final @NotNull Callable callable) * @param - the result type of the {@link Supplier} */ public static Supplier wrapSupplier(final @NotNull Supplier supplier) { - final IHub oldState = Sentry.getCurrentHub(); - final IHub newHub = oldState.clone(); + + final IHub newHub = Sentry.getCurrentHub().clone(); return () -> { + final IHub oldState = Sentry.getCurrentHub(); Sentry.setCurrentHub(newHub); try { return supplier.get(); diff --git a/sentry/src/test/java/io/sentry/SentryWrapperTest.kt b/sentry/src/test/java/io/sentry/SentryWrapperTest.kt index 9b3176393e..7f6d449eac 100644 --- a/sentry/src/test/java/io/sentry/SentryWrapperTest.kt +++ b/sentry/src/test/java/io/sentry/SentryWrapperTest.kt @@ -1,24 +1,67 @@ package io.sentry import java.util.concurrent.CompletableFuture +import java.util.concurrent.ExecutorService import java.util.concurrent.Executors import kotlin.test.AfterTest import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertNotEquals class SentryWrapperTest { private val dsn = "http://key@localhost/proj" - private val executor = Executors.newSingleThreadExecutor() + private lateinit var executor: ExecutorService @BeforeTest - @AfterTest fun beforeTest() { + executor = Executors.newSingleThreadExecutor() + } + + @AfterTest + fun afterTest() { + executor.shutdown() Sentry.close() SentryCrashLastRunState.getInstance().reset() } + @Test + fun `hub is reset to its state within the thread after supply is done`() { + Sentry.init { + it.dsn = dsn + it.beforeSend = SentryOptions.BeforeSendCallback { event, hint -> + event + } + } + + val mainHub = Sentry.getCurrentHub() + val threadedHub = Sentry.getCurrentHub().clone() + + executor.submit { + Sentry.setCurrentHub(threadedHub) + }.get() + + assertEquals(mainHub, Sentry.getCurrentHub()) + + val callableFuture = + CompletableFuture.supplyAsync( + SentryWrapper.wrapSupplier { + assertNotEquals(mainHub, Sentry.getCurrentHub()) + assertNotEquals(threadedHub, Sentry.getCurrentHub()) + "Result 1" + }, + executor + ) + + callableFuture.join() + + executor.submit { + assertNotEquals(mainHub, Sentry.getCurrentHub()) + assertEquals(threadedHub, Sentry.getCurrentHub()) + }.get() + } + @Test fun `wrapped supply async isolates Hubs`() { val capturedEvents = mutableListOf() @@ -119,4 +162,36 @@ class SentryWrapperTest { assertEquals(2, clonedEvent?.breadcrumbs?.size) assertEquals(2, clonedEvent2?.breadcrumbs?.size) } + + @Test + fun `hub is reset to its state within the thread after callable is done`() { + Sentry.init { + it.dsn = dsn + } + + val mainHub = Sentry.getCurrentHub() + val threadedHub = Sentry.getCurrentHub().clone() + + executor.submit { + Sentry.setCurrentHub(threadedHub) + }.get() + + assertEquals(mainHub, Sentry.getCurrentHub()) + + val callableFuture = + executor.submit( + SentryWrapper.wrapCallable { + assertNotEquals(mainHub, Sentry.getCurrentHub()) + assertNotEquals(threadedHub, Sentry.getCurrentHub()) + "Result 1" + } + ) + + callableFuture.get() + + executor.submit { + assertNotEquals(mainHub, Sentry.getCurrentHub()) + assertEquals(threadedHub, Sentry.getCurrentHub()) + }.get() + } }