diff --git a/CHANGELOG.md b/CHANGELOG.md index ba7522abacb..ee39c4c11b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## Unreleased +### Bugfixes + +* Make closing scope idempotent and non-operational when corresponding context is not current. + [(#5061)](https://github.com/open-telemetry/opentelemetry-java/pull/5061) +* ## Version 1.21.0 (2022-12-09) ### API diff --git a/context/src/main/java/io/opentelemetry/context/ThreadLocalContextStorage.java b/context/src/main/java/io/opentelemetry/context/ThreadLocalContextStorage.java index 92f24849a29..917eed2e088 100644 --- a/context/src/main/java/io/opentelemetry/context/ThreadLocalContextStorage.java +++ b/context/src/main/java/io/opentelemetry/context/ThreadLocalContextStorage.java @@ -30,14 +30,30 @@ public Scope attach(Context toAttach) { THREAD_LOCAL_STORAGE.set(toAttach); - return () -> { - if (current() != toAttach) { + return new ScopeImpl(beforeAttach, toAttach); + } + + private class ScopeImpl implements Scope { + @Nullable private final Context beforeAttach; + private final Context toAttach; + private boolean closed; + + private ScopeImpl(@Nullable Context beforeAttach, Context toAttach) { + this.beforeAttach = beforeAttach; + this.toAttach = toAttach; + } + + @Override + public void close() { + if (!closed && current() == toAttach) { + closed = true; + THREAD_LOCAL_STORAGE.set(beforeAttach); + } else { logger.log( Level.FINE, - "Context in storage not the expected context, Scope.close was not called correctly"); + " Trying to close scope which does not represent current context. Ignoring the call."); } - THREAD_LOCAL_STORAGE.set(beforeAttach); - }; + } } @Override diff --git a/context/src/test/java/io/opentelemetry/context/ContextTest.java b/context/src/test/java/io/opentelemetry/context/ContextTest.java index 2ee5e01ab18..d8a7a0b7bf8 100644 --- a/context/src/test/java/io/opentelemetry/context/ContextTest.java +++ b/context/src/test/java/io/opentelemetry/context/ContextTest.java @@ -118,7 +118,7 @@ void newThreadStartsWithRoot() throws Exception { } @Test - public void closingScopeWhenNotActiveIsLogged() { + public void closingScopeWhenNotActiveIsNoopAndLogged() { Context initial = Context.current(); Context context = initial.with(ANIMAL, "cat"); try (Scope scope = context.makeCurrent()) { @@ -126,13 +126,34 @@ public void closingScopeWhenNotActiveIsLogged() { try (Scope ignored = context2.makeCurrent()) { assertThat(Context.current().get(ANIMAL)).isEqualTo("dog"); scope.close(); + assertThat(Context.current().get(ANIMAL)).isEqualTo("dog"); } } assertThat(Context.current()).isEqualTo(initial); - LoggingEvent log = logs.assertContains("Context in storage not the expected context"); + LoggingEvent log = + logs.assertContains("Trying to close scope which does not represent current context"); assertThat(log.getLevel()).isEqualTo(Level.DEBUG); } + @SuppressWarnings("MustBeClosedChecker") + @Test + public void closeScopeIsIdempotent() { + Context initial = Context.current(); + Context context1 = Context.root().with(ANIMAL, "cat"); + Scope scope1 = context1.makeCurrent(); + Context context2 = context1.with(ANIMAL, "dog"); + Scope scope2 = context2.makeCurrent(); + + scope2.close(); + assertThat(Context.current()).isEqualTo(context1); + + scope1.close(); + assertThat(Context.current()).isEqualTo(initial); + + scope2.close(); + assertThat(Context.current()).isEqualTo(initial); + } + @Test void withValues() { Context context1 = Context.current().with(ANIMAL, "cat");