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 <[email protected]> * code review --------- Co-authored-by: Alexander Dinauer <[email protected]>
getsentry · Feb 26, 2024 · aa91b8f · aa91b8f
1 parent bf82eb3
commit aa91b8f
Show file tree

Hide file tree

Showing 11 changed files with 330 additions and 10 deletions.
diff --git 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
@@ -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
@@ -14,9 +14,10 @@
 public final class SentryScheduleHook implements Function<Runnable, Runnable> {
   @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
@@ -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/...spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/webflux/SentryScheduleHookTest.kt b/...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
@@ -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
@@ -14,9 +14,10 @@
 public final class SentryScheduleHook implements Function<Runnable, Runnable> {
   @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
@@ -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
@@ -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
@@ -28,10 +28,10 @@ public final class SentryWrapper {
    * @param <U> - the result type of the {@link Callable}
    */
   public static <U> Callable<U> wrapCallable(final @NotNull Callable<U> 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 <U> Callable<U> wrapCallable(final @NotNull Callable<U> callable)
    * @param <U> - the result type of the {@link Supplier}
    */
   public static <U> Supplier<U> wrapSupplier(final @NotNull Supplier<U> 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();