SentryTaskDecorator cleanup

[micopiira]

Integration

sentry

Java Version

17

Version

7.2.0

Steps to Reproduce

Shouldn't the SentryTaskDecorator, SentryWrapper, SentryScheduleHook implementations capture the previous state inside the executing thread and not the calling thread? For example as seen here: https://bsideup.github.io/posts/daily_reactive/thread_locals/

The implementation I'm suggesting would then look like this:

final IHub newHub = Sentry.getCurrentHub().clone();
return () -> {
	final IHub oldState = Sentry.getCurrentHub();
	Sentry.setCurrentHub(newHub);
	try {
		runnable.run();
	} finally {
		Sentry.setCurrentHub(oldState);
	}
};

Here is a code sample reproducing the problem.

final ExecutorService executorService = Executors.newSingleThreadExecutor();
final TaskDecorator sentryTaskDecorator = new SentryTaskDecorator();

final IHub hubA = new FakeHub("a");
final IHub hubB = new FakeHub("b");

// Here we set pool-1-thread-1 hub to hubA.
executorService.submit(() -> {
	Sentry.setCurrentHub(hubA);
	System.out.println(Thread.currentThread().getName() + " thread hub is now: " + ((FakeHub) Sentry.getCurrentHub()).getName());
});

// Set main thread hub to hubB that will then get propagated by the SentryTaskDecorator
Sentry.setCurrentHub(hubB);
System.out.println(Thread.currentThread().getName() + " thread hub is now: " + ((FakeHub) Sentry.getCurrentHub()).getName());

// Here we see that the hubB is correctly propagated to pool-1-thread-1
executorService.submit(sentryTaskDecorator.decorate(() -> {
	System.out.println(Thread.currentThread().getName() + " thread hub is now: " + ((FakeHub) Sentry.getCurrentHub()).getName());
}));

// Here we see that pool-1-thread-1 is still hubB even though it should be restored to hubA that it was before the decorated call!
executorService.submit(() -> {
	System.out.println(Thread.currentThread().getName() + " thread hub is now: " + ((FakeHub) Sentry.getCurrentHub()).getName());
});


executorService.shutdown();

Expected Result

pool-1-thread-1 thread hub is now: a
main thread hub is now: b
pool-1-thread-1 thread hub is now: b
pool-1-thread-1 thread hub is now: a

Actual Result

pool-1-thread-1 thread hub is now: a
main thread hub is now: b
pool-1-thread-1 thread hub is now: b
pool-1-thread-1 thread hub is now: b

I'm also more than happy to provide a PR for this issue if you at Sentry agree with me on this problem.

[micopiira]

As a side note, when using the new ContextPropagatingTaskDecorator together with SentryReactorThreadLocalAccessor the result is as expected.

final TaskDecorator sentryTaskDecorator = new ContextPropagatingTaskDecorator(
		ContextSnapshotFactory.builder()
				.contextRegistry(new ContextRegistry()
						.registerThreadLocalAccessor(new SentryReactorThreadLocalAccessor()))
				.build()
);

[adinauer]

@micopiira thanks for opening this issue and doing investigations. This seems like an oversight on our part and should be fixed. We'll take a look and probably open a PR to fix soon.