From e96271151c76b6bb9f54e7d529cb874da7e3f498 Mon Sep 17 00:00:00 2001 From: Liudmila Molkova Date: Wed, 21 Dec 2022 19:35:32 -0800 Subject: [PATCH 1/3] Make closing scope idempotent and noop when different context is active --- CHANGELOG.md | 5 +++ .../context/ThreadLocalContextStorage.java | 27 +++++++++++++--- .../io/opentelemetry/context/ContextTest.java | 31 +++++++++++++++++-- 3 files changed, 56 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ba7522abacb..f0df326e1cb 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. + [(#TODO)](https://github.com/open-telemetry/opentelemetry-java/pull/TODO) +* ## 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..1fae45a73e3 100644 --- a/context/src/main/java/io/opentelemetry/context/ThreadLocalContextStorage.java +++ b/context/src/main/java/io/opentelemetry/context/ThreadLocalContextStorage.java @@ -7,6 +7,7 @@ import java.util.logging.Level; import java.util.logging.Logger; +import javax.annotation.Nonnull; import javax.annotation.Nullable; enum ThreadLocalContextStorage implements ContextStorage { @@ -30,14 +31,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, @Nonnull 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..23a25321315 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,40 @@ 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); + + scope2.close(); + assertThat(Context.current()).isEqualTo(context1); + + scope1.close(); + assertThat(Context.current()).isEqualTo(initial); + + scope1.close(); + assertThat(Context.current()).isEqualTo(initial); + + scope2.close(); + assertThat(Context.current()).isEqualTo(initial); + } + @Test void withValues() { Context context1 = Context.current().with(ANIMAL, "cat"); From 295f3489861c4cbd927d5b2148c23496fa42be89 Mon Sep 17 00:00:00 2001 From: Liudmila Molkova Date: Wed, 21 Dec 2022 19:39:26 -0800 Subject: [PATCH 2/3] changelog --- CHANGELOG.md | 2 +- .../src/test/java/io/opentelemetry/context/ContextTest.java | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f0df326e1cb..ee39c4c11b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ ### Bugfixes * Make closing scope idempotent and non-operational when corresponding context is not current. - [(#TODO)](https://github.com/open-telemetry/opentelemetry-java/pull/TODO) + [(#5061)](https://github.com/open-telemetry/opentelemetry-java/pull/5061) * ## Version 1.21.0 (2022-12-09) diff --git a/context/src/test/java/io/opentelemetry/context/ContextTest.java b/context/src/test/java/io/opentelemetry/context/ContextTest.java index 23a25321315..d8a7a0b7bf8 100644 --- a/context/src/test/java/io/opentelemetry/context/ContextTest.java +++ b/context/src/test/java/io/opentelemetry/context/ContextTest.java @@ -147,12 +147,6 @@ public void closeScopeIsIdempotent() { scope2.close(); assertThat(Context.current()).isEqualTo(context1); - scope2.close(); - assertThat(Context.current()).isEqualTo(context1); - - scope1.close(); - assertThat(Context.current()).isEqualTo(initial); - scope1.close(); assertThat(Context.current()).isEqualTo(initial); From 87df1448147b079b0e9aca00dde3a824665501ee Mon Sep 17 00:00:00 2001 From: Liudmila Molkova Date: Thu, 22 Dec 2022 12:08:39 -0800 Subject: [PATCH 3/3] notnull --- .../io/opentelemetry/context/ThreadLocalContextStorage.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/context/src/main/java/io/opentelemetry/context/ThreadLocalContextStorage.java b/context/src/main/java/io/opentelemetry/context/ThreadLocalContextStorage.java index 1fae45a73e3..917eed2e088 100644 --- a/context/src/main/java/io/opentelemetry/context/ThreadLocalContextStorage.java +++ b/context/src/main/java/io/opentelemetry/context/ThreadLocalContextStorage.java @@ -7,7 +7,6 @@ import java.util.logging.Level; import java.util.logging.Logger; -import javax.annotation.Nonnull; import javax.annotation.Nullable; enum ThreadLocalContextStorage implements ContextStorage { @@ -39,7 +38,7 @@ private class ScopeImpl implements Scope { private final Context toAttach; private boolean closed; - private ScopeImpl(@Nullable Context beforeAttach, @Nonnull Context toAttach) { + private ScopeImpl(@Nullable Context beforeAttach, Context toAttach) { this.beforeAttach = beforeAttach; this.toAttach = toAttach; }