From 1d648275d8073d2fbfeb0f55fc7829ca908c59f9 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Tue, 26 Apr 2022 22:21:36 +0700 Subject: [PATCH 01/44] Add TraceAbleRequestContextStorage for ease of detect context leak --- .../TraceAbleRequestContextStorage.java | 94 ++++++++ .../common/ServiceRequestContextTest.java | 210 ++++++++++++++++ .../common/TraceRequestContextLeakTest.java | 227 ++++++++++++++++++ .../TraceRequestContextStorageProvider.java | 25 ++ ...meria.common.RequestContextStorageProvider | 1 + settings.gradle | 1 + 6 files changed, 558 insertions(+) create mode 100644 core/src/main/java/com/linecorp/armeria/common/TraceAbleRequestContextStorage.java create mode 100644 it/trace-context-leak/src/test/java/com/linecorp/armeria/common/ServiceRequestContextTest.java create mode 100644 it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java create mode 100644 it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextStorageProvider.java create mode 100644 it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.RequestContextStorageProvider diff --git a/core/src/main/java/com/linecorp/armeria/common/TraceAbleRequestContextStorage.java b/core/src/main/java/com/linecorp/armeria/common/TraceAbleRequestContextStorage.java new file mode 100644 index 00000000000..eb07e48bb79 --- /dev/null +++ b/core/src/main/java/com/linecorp/armeria/common/TraceAbleRequestContextStorage.java @@ -0,0 +1,94 @@ +/* + * Copyright 2022 LINE Corporation + * + * LINE Corporation licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package com.linecorp.armeria.common; + +import static java.lang.Thread.currentThread; +import static java.util.Objects.requireNonNull; + +import com.linecorp.armeria.client.ClientRequestContext; +import com.linecorp.armeria.common.annotation.Nullable; +import com.linecorp.armeria.server.ServiceRequestContext; + +import io.netty.util.concurrent.FastThreadLocal; + +/** + * A {@link RequestContextStorage} which keeps track of {@link RequestContext}s, reporting pushed thread + * information if a {@link RequestContext} is leaked. + */ +public final class TraceAbleRequestContextStorage implements RequestContextStorage { + + private final RequestContextStorage delegate; + private final FastThreadLocal pendingRequestCtx; + + /** + * Creates a new instance. + * @param delegate the underlying {@link RequestContextStorage} that stores {@link RequestContext} + */ + public TraceAbleRequestContextStorage(RequestContextStorage delegate) { + this.delegate = delegate; + pendingRequestCtx = new FastThreadLocal<>(); + } + + @Nullable + @Override + public T push(RequestContext toPush) { + requireNonNull(toPush, "toPush"); + + final RequestContext prevContext = delegate.currentOrNull(); + if (prevContext != null) { + if (prevContext == toPush) { + // Re-entrance + } else if (toPush instanceof ServiceRequestContext && + prevContext.root() == toPush) { + // The delegate has the ServiceRequestContext whose root() is toPush + } else if (toPush instanceof ClientRequestContext && + prevContext.root() == toPush.root()) { + // The delegate has the ClientRequestContext whose root() is the same toPush.root() + } else { + throw pendingRequestCtx.get(); + } + } + pendingRequestCtx.set(new PendingRequestContextStackTrace(toPush)); + + return delegate.push(toPush); + } + + @Override + public void pop(RequestContext current, @Nullable RequestContext toRestore) { + delegate.pop(current, toRestore); + pendingRequestCtx.remove(); + } + + @Nullable + @Override + public T currentOrNull() { + return delegate.currentOrNull(); + } + + private static class PendingRequestContextStackTrace extends IllegalStateException { + + private static final long serialVersionUID = -689451606253441556L; + + final RequestContext context; + + PendingRequestContextStackTrace(RequestContext context) { + super("At thread [" + currentThread().getName() + "], previous RequestContext didn't popped : " + + context + ", It is pushed at the following stacktrace"); + this.context = context; + } + } +} diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/ServiceRequestContextTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/ServiceRequestContextTest.java new file mode 100644 index 00000000000..760ed5bdc70 --- /dev/null +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/ServiceRequestContextTest.java @@ -0,0 +1,210 @@ +/* + * Copyright 2019 LINE Corporation + * + * LINE Corporation licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package com.linecorp.armeria.common; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.util.function.Function; + +import org.junit.jupiter.api.Test; + +import com.google.common.collect.ImmutableList; + +import com.linecorp.armeria.client.ClientRequestContext; +import com.linecorp.armeria.common.annotation.Nullable; +import com.linecorp.armeria.common.util.SafeCloseable; +import com.linecorp.armeria.server.ServiceRequestContext; + +/** + * This is copy from ServiceRequestContextTest on core module to test behaviour consistency. + */ +class ServiceRequestContextTest { + + @Test + void current() { + assertThatThrownBy(ServiceRequestContext::current).isInstanceOf(IllegalStateException.class) + .hasMessageContaining("unavailable"); + + final ServiceRequestContext sctx = serviceRequestContext(); + try (SafeCloseable unused = sctx.push()) { + assertThat(ServiceRequestContext.current()).isSameAs(sctx); + final ClientRequestContext cctx = clientRequestContext(); + try (SafeCloseable unused1 = cctx.push()) { + assertThat(ServiceRequestContext.current()).isSameAs(sctx); + assertThat(ClientRequestContext.current()).isSameAs(cctx); + assertThat((ClientRequestContext) RequestContext.current()).isSameAs(cctx); + } + assertCurrentCtx(sctx); + } + assertCurrentCtx(null); + + try (SafeCloseable unused = clientRequestContext().push()) { + assertThatThrownBy(ServiceRequestContext::current) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("not a server-side context"); + } + } + + @Test + void currentOrNull() { + assertThat(ServiceRequestContext.currentOrNull()).isNull(); + + final ServiceRequestContext sctx = serviceRequestContext(); + try (SafeCloseable unused = sctx.push()) { + assertThat(ServiceRequestContext.currentOrNull()).isSameAs(sctx); + final ClientRequestContext cctx = clientRequestContext(); + try (SafeCloseable unused1 = cctx.push()) { + assertThat(ServiceRequestContext.currentOrNull()).isSameAs(sctx); + assertThat(ClientRequestContext.current()).isSameAs(cctx); + assertThat((ClientRequestContext) RequestContext.current()).isSameAs(cctx); + } + assertCurrentCtx(sctx); + } + assertCurrentCtx(null); + + try (SafeCloseable unused = clientRequestContext().push()) { + assertThat(ServiceRequestContext.currentOrNull()).isNull(); + } + } + + @Test + void mapCurrent() { + assertThat(ServiceRequestContext.mapCurrent(ctx -> "foo", () -> "defaultValue")) + .isEqualTo("defaultValue"); + assertThat(ServiceRequestContext.mapCurrent(Function.identity(), null)).isNull(); + + final ServiceRequestContext sctx = serviceRequestContext(); + try (SafeCloseable unused = sctx.push()) { + assertThat(ServiceRequestContext.mapCurrent(c -> c == sctx ? "foo" : "bar", + () -> "defaultValue")) + .isEqualTo("foo"); + assertThat(ServiceRequestContext.mapCurrent(Function.identity(), null)).isSameAs(sctx); + final ClientRequestContext cctx = clientRequestContext(); + try (SafeCloseable unused1 = cctx.push()) { + assertThat(ServiceRequestContext.mapCurrent(c -> c == sctx ? "foo" : "bar", + () -> "defaultValue")) + .isEqualTo("foo"); + assertThat(ClientRequestContext.mapCurrent(c -> c == cctx ? "baz" : "qux", + () -> "defaultValue")) + .isEqualTo("baz"); + assertThat(ServiceRequestContext.mapCurrent(Function.identity(), null)).isSameAs(sctx); + assertThat(ClientRequestContext.mapCurrent(Function.identity(), null)).isSameAs(cctx); + assertThat(RequestContext.mapCurrent(Function.identity(), null)).isSameAs(cctx); + } + assertCurrentCtx(sctx); + } + assertCurrentCtx(null); + + try (SafeCloseable unused = clientRequestContext().push()) { + assertThatThrownBy(() -> ServiceRequestContext.mapCurrent(c -> "foo", () -> "bar")) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("not a server-side context"); + } + } + + @Test + void pushReentrance() { + final ServiceRequestContext ctx = serviceRequestContext(); + try (SafeCloseable ignored = ctx.push()) { + assertCurrentCtx(ctx); + try (SafeCloseable ignored2 = ctx.push()) { + assertCurrentCtx(ctx); + } + assertCurrentCtx(ctx); + } + assertCurrentCtx(null); + } + + @Test + void pushWithOldClientCtxWhoseRootIsThisServiceCtx() { + final ServiceRequestContext sctx = serviceRequestContext(); + try (SafeCloseable ignored = sctx.push()) { + assertCurrentCtx(sctx); + // The root of ClientRequestContext is sctx. + final ClientRequestContext cctx = clientRequestContext(); + try (SafeCloseable ignored1 = cctx.push()) { + assertCurrentCtx(cctx); + try (SafeCloseable ignored2 = sctx.push()) { + assertCurrentCtx(sctx); + } + assertCurrentCtx(cctx); + } + assertCurrentCtx(sctx); + } + assertCurrentCtx(null); + } + + @Test + void pushWithOldIrrelevantClientCtx() { + final ClientRequestContext cctx = clientRequestContext(); + try (SafeCloseable ignored = cctx.push()) { + assertCurrentCtx(cctx); + final ServiceRequestContext sctx = serviceRequestContext(); + assertThatThrownBy(sctx::push).isInstanceOf(IllegalStateException.class); + } + assertCurrentCtx(null); + } + + @Test + void pushWithOldIrrelevantServiceCtx() { + final ServiceRequestContext sctx1 = serviceRequestContext(); + final ServiceRequestContext sctx2 = serviceRequestContext(); + try (SafeCloseable ignored = sctx1.push()) { + assertCurrentCtx(sctx1); + assertThatThrownBy(sctx2::push).isInstanceOf(IllegalStateException.class); + } + assertCurrentCtx(null); + } + + @Test + void queryParams() { + final String path = "/foo"; + final QueryParams queryParams = QueryParams.of("param1", "value1", + "param1", "value2", + "Param1", "Value3", + "PARAM1", "VALUE4"); + final String pathAndQuery = path + '?' + queryParams.toQueryString(); + final ServiceRequestContext ctx = ServiceRequestContext.of(HttpRequest.of(HttpMethod.GET, + pathAndQuery)); + + assertThat(ctx.queryParams()).isEqualTo(queryParams); + + assertThat(ctx.queryParam("param1")).isEqualTo("value1"); + assertThat(ctx.queryParam("Param1")).isEqualTo("Value3"); + assertThat(ctx.queryParam("PARAM1")).isEqualTo("VALUE4"); + assertThat(ctx.queryParam("Not exist")).isNull(); + + assertThat(ctx.queryParams("param1")).isEqualTo(ImmutableList.of("value1", "value2")); + assertThat(ctx.queryParams("Param1")).isEqualTo(ImmutableList.of("Value3")); + assertThat(ctx.queryParams("PARAM1")).isEqualTo(ImmutableList.of("VALUE4")); + assertThat(ctx.queryParams("Not exist")).isEmpty(); + } + + private static void assertCurrentCtx(@Nullable RequestContext ctx) { + final RequestContext current = RequestContext.currentOrNull(); + assertThat(current).isSameAs(ctx); + } + + private static ServiceRequestContext serviceRequestContext() { + return ServiceRequestContext.of(HttpRequest.of(HttpMethod.GET, "/")); + } + + private static ClientRequestContext clientRequestContext() { + return ClientRequestContext.of(HttpRequest.of(HttpMethod.GET, "/")); + } +} diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java new file mode 100644 index 00000000000..cf2aff70268 --- /dev/null +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java @@ -0,0 +1,227 @@ +/* + * Copyright 2022 LINE Corporation + * + * LINE Corporation licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package com.linecorp.armeria.common; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.Executor; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import com.linecorp.armeria.common.util.SafeCloseable; +import com.linecorp.armeria.server.ServiceRequestContext; +import com.linecorp.armeria.testing.junit5.common.EventLoopExtension; +import com.linecorp.armeria.testing.junit5.common.EventLoopGroupExtension; + +import io.netty.channel.EventLoop; +import io.netty.channel.EventLoopGroup; + +class TraceRequestContextLeakTest { + + @RegisterExtension + static final EventLoopExtension eventLoopExtension = new EventLoopExtension(); + + @RegisterExtension + static final EventLoopGroupExtension eventLoopGroupExtension = new EventLoopGroupExtension(2); + + @Test + void singleThreadContextNotLeak() throws InterruptedException { + final AtomicBoolean isThrown = new AtomicBoolean(false); + final CountDownLatch latch = new CountDownLatch(2); + + final EventLoop executor = eventLoopExtension.get(); + + executor.execute(() -> { + final ServiceRequestContext ctx = newCtx("/1"); + try (SafeCloseable ignore = ctx.push()) { + //ignore + } catch (Exception ex) { + isThrown.set(true); + } finally { + latch.countDown(); + } + }); + + executor.execute(() -> { + final ServiceRequestContext anotherCtx = newCtx("/2"); + try (SafeCloseable ignore = anotherCtx.push()) { + //ignore + } catch (Exception ex) { + isThrown.set(true); + } finally { + latch.countDown(); + } + }); + + latch.await(); + assertThat(isThrown).isFalse(); + } + + @Test + @SuppressWarnings("MustBeClosedChecker") + void singleTreadContextLeak() throws InterruptedException { + final AtomicBoolean isThrown = new AtomicBoolean(); + final AtomicReference exception = new AtomicReference<>(); + final CountDownLatch latch = new CountDownLatch(2); + final DeferredClose deferredClose = new DeferredClose(); + + final EventLoop executor = eventLoopExtension.get(); + + executor.execute(() -> { + final ServiceRequestContext ctx = newCtx("/1"); + final SafeCloseable leaked = ctx.push(); + latch.countDown(); + deferredClose.add(executor, leaked); + }); + + executor.execute(() -> { + final ServiceRequestContext anotherCtx = newCtx("/2"); + try (SafeCloseable ignore = anotherCtx.push()) { + //ignore + } catch (Exception ex) { + isThrown.set(true); + exception.set(ex); + } finally { + latch.countDown(); + } + }); + + latch.await(); + assertThat(isThrown).isTrue(); + assertThat(exception.get()).hasMessageContaining("It is pushed at the following stacktrace"); + + deferredClose.closeAll(); + } + + @Test + @SuppressWarnings("MustBeClosedChecker") + void multiThreadContextNotLeak() throws InterruptedException { + final AtomicBoolean isThrown = new AtomicBoolean(false); + final CountDownLatch latch = new CountDownLatch(2); + final DeferredClose deferredClose = new DeferredClose(); + + final EventLoopGroup executor = eventLoopGroupExtension.get(); + + executor.next().execute(() -> { + final ServiceRequestContext ctx = newCtx("/1"); + final SafeCloseable leaked = ctx.push(); + latch.countDown(); + deferredClose.add(executor, leaked); + }); + + executor.next().execute(() -> { + //Leak happened on the first eventLoop doesn't make eventLoop when trying to push + final ServiceRequestContext anotherCtx = newCtx("/2"); + try (SafeCloseable ignore = anotherCtx.push()) { + //ignore + } catch (Exception ex) { + //Not suppose to throw exception on the second event loop + isThrown.set(true); + } finally { + latch.countDown(); + } + }); + + latch.await(); + assertThat(isThrown).isFalse(); + + deferredClose.closeAll(); + } + + @Test + @SuppressWarnings("MustBeClosedChecker") + void multiThreadContextLeak() throws InterruptedException { + final AtomicBoolean isThrown = new AtomicBoolean(false); + final AtomicReference exception = new AtomicReference<>(); + final CountDownLatch latch = new CountDownLatch(3); + final DeferredClose deferredClose = new DeferredClose(); + + final EventLoopGroup executor = eventLoopGroupExtension.get(); + + final ServiceRequestContext ctx = newCtx("/1"); + final ServiceRequestContext anotherCtx = newCtx("/2"); + + final Executor ex1 = executor.next(); + final Executor ex2 = executor.next(); + + ex1.execute(() -> { + final SafeCloseable leaked = ctx.push(); + latch.countDown(); + deferredClose.add(ex1, leaked); + }); + + ex2.execute(() -> { + try { + //intentional leak + final SafeCloseable leaked = anotherCtx.push(); + deferredClose.add(ex2, leaked); + } catch (Exception ex) { + isThrown.set(true); + } finally { + latch.countDown(); + } + }); + + assertThat(isThrown).isFalse(); + + ex1.execute(() -> { + try (SafeCloseable ignore = anotherCtx.push()) { + //ignore + } catch (Exception ex) { + isThrown.set(true); + exception.set(ex); + } finally { + latch.countDown(); + } + }); + + latch.await(); + assertThat(isThrown).isTrue(); + assertThat(exception.get()).hasMessageContaining("It is pushed at the following stacktrace"); + + deferredClose.closeAll(); + } + + private static ServiceRequestContext newCtx(String path) { + return ServiceRequestContext.builder(HttpRequest.of(HttpMethod.GET, path)) + .build(); + } + + //Utility to clean up RequestContext leak after test + private static class DeferredClose { + + private final AtomicReference> toClose; + + DeferredClose() { + toClose = new AtomicReference<>(new ConcurrentHashMap<>()); + } + + void add(Executor executor, SafeCloseable closeable) { + toClose.get().put(executor, closeable); + } + + void closeAll() { + toClose.get().forEach((executor, closeable) -> executor.execute(closeable::close)); + } + } +} diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextStorageProvider.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextStorageProvider.java new file mode 100644 index 00000000000..3fe34ff0c56 --- /dev/null +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextStorageProvider.java @@ -0,0 +1,25 @@ +/* + * Copyright 2022 LINE Corporation + * + * LINE Corporation licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package com.linecorp.armeria.common; + +public final class TraceRequestContextStorageProvider implements RequestContextStorageProvider { + + @Override + public RequestContextStorage newStorage() { + return new TraceAbleRequestContextStorage(RequestContextStorage.threadLocal()); + } +} diff --git a/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.RequestContextStorageProvider b/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.RequestContextStorageProvider new file mode 100644 index 00000000000..eb793a88cdc --- /dev/null +++ b/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.RequestContextStorageProvider @@ -0,0 +1 @@ +com.linecorp.armeria.common.TraceRequestContextStorageProvider diff --git a/settings.gradle b/settings.gradle index 5d8eee9ab0e..ce931bb875a 100644 --- a/settings.gradle +++ b/settings.gradle @@ -96,6 +96,7 @@ includeWithFlags ':it:spring:boot2-tomcat8', 'java', 'relocate' includeWithFlags ':it:spring:boot2-tomcat9', 'java', 'relocate' includeWithFlags ':it:spring:webflux-security', 'java', 'relocate' includeWithFlags ':it:thrift-fullcamel', 'java', 'relocate' +includeWithFlags ':it:trace-context-leak', 'java', 'relocate' includeWithFlags ':jetty9.3', 'java', 'relocate' includeWithFlags ':testing-internal', 'java', 'relocate' includeWithFlags ':thrift0.12', 'java', 'relocate' From c79b852dd37c3364d4d0fdccb5cc4960fbb19195 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Thu, 28 Apr 2022 17:39:30 +0700 Subject: [PATCH 02/44] Update classname --- ...textStorage.java => LeakTracingRequestContextStorage.java} | 4 ++-- ...der.java => LeakTracingRequestContextStorageProvider.java} | 4 ++-- .../com.linecorp.armeria.common.RequestContextStorageProvider | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) rename core/src/main/java/com/linecorp/armeria/common/{TraceAbleRequestContextStorage.java => LeakTracingRequestContextStorage.java} (95%) rename it/trace-context-leak/src/test/java/com/linecorp/armeria/common/{TraceRequestContextStorageProvider.java => LeakTracingRequestContextStorageProvider.java} (79%) diff --git a/core/src/main/java/com/linecorp/armeria/common/TraceAbleRequestContextStorage.java b/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java similarity index 95% rename from core/src/main/java/com/linecorp/armeria/common/TraceAbleRequestContextStorage.java rename to core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java index eb07e48bb79..cf1c3af4baf 100644 --- a/core/src/main/java/com/linecorp/armeria/common/TraceAbleRequestContextStorage.java +++ b/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java @@ -29,7 +29,7 @@ * A {@link RequestContextStorage} which keeps track of {@link RequestContext}s, reporting pushed thread * information if a {@link RequestContext} is leaked. */ -public final class TraceAbleRequestContextStorage implements RequestContextStorage { +public final class LeakTracingRequestContextStorage implements RequestContextStorage { private final RequestContextStorage delegate; private final FastThreadLocal pendingRequestCtx; @@ -38,7 +38,7 @@ public final class TraceAbleRequestContextStorage implements RequestContextStora * Creates a new instance. * @param delegate the underlying {@link RequestContextStorage} that stores {@link RequestContext} */ - public TraceAbleRequestContextStorage(RequestContextStorage delegate) { + public LeakTracingRequestContextStorage(RequestContextStorage delegate) { this.delegate = delegate; pendingRequestCtx = new FastThreadLocal<>(); } diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextStorageProvider.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/LeakTracingRequestContextStorageProvider.java similarity index 79% rename from it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextStorageProvider.java rename to it/trace-context-leak/src/test/java/com/linecorp/armeria/common/LeakTracingRequestContextStorageProvider.java index 3fe34ff0c56..6257976b45c 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextStorageProvider.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/LeakTracingRequestContextStorageProvider.java @@ -16,10 +16,10 @@ package com.linecorp.armeria.common; -public final class TraceRequestContextStorageProvider implements RequestContextStorageProvider { +public final class LeakTracingRequestContextStorageProvider implements RequestContextStorageProvider { @Override public RequestContextStorage newStorage() { - return new TraceAbleRequestContextStorage(RequestContextStorage.threadLocal()); + return new LeakTracingRequestContextStorage(RequestContextStorage.threadLocal()); } } diff --git a/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.RequestContextStorageProvider b/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.RequestContextStorageProvider index eb793a88cdc..52903537ec6 100644 --- a/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.RequestContextStorageProvider +++ b/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.RequestContextStorageProvider @@ -1 +1 @@ -com.linecorp.armeria.common.TraceRequestContextStorageProvider +com.linecorp.armeria.common.LeakTracingRequestContextStorageProvider From 443e740fa6d4e113146374180cc0d36d773dd95e Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Fri, 29 Apr 2022 00:29:37 +0700 Subject: [PATCH 03/44] Add Sampler to constructor of LeakTracingRequestContextStorage --- .../LeakTracingRequestContextStorage.java | 30 ++++++++++++++----- .../common/ServiceRequestContextTest.java | 5 ++-- .../common/TraceRequestContextLeakTest.java | 4 +-- 3 files changed, 28 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java b/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java index cf1c3af4baf..0b1a8f7508e 100644 --- a/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java +++ b/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java @@ -21,6 +21,7 @@ import com.linecorp.armeria.client.ClientRequestContext; import com.linecorp.armeria.common.annotation.Nullable; +import com.linecorp.armeria.common.util.Sampler; import com.linecorp.armeria.server.ServiceRequestContext; import io.netty.util.concurrent.FastThreadLocal; @@ -33,14 +34,26 @@ public final class LeakTracingRequestContextStorage implements RequestContextSto private final RequestContextStorage delegate; private final FastThreadLocal pendingRequestCtx; + private final Sampler> sampler; /** * Creates a new instance. * @param delegate the underlying {@link RequestContextStorage} that stores {@link RequestContext} */ public LeakTracingRequestContextStorage(RequestContextStorage delegate) { + this(delegate, Flags.verboseExceptionSampler()); + } + + /** + * Creates a new instance. + * @param delegate the underlying {@link RequestContextStorage} that stores {@link RequestContext} + * @param sampler the {@link Sampler} that determines whether to retain the stacktrace of the context leaks + */ + public LeakTracingRequestContextStorage(RequestContextStorage delegate, + Sampler> sampler) { this.delegate = delegate; pendingRequestCtx = new FastThreadLocal<>(); + this.sampler = sampler; } @Nullable @@ -62,7 +75,12 @@ public T push(RequestContext toPush) { throw pendingRequestCtx.get(); } } - pendingRequestCtx.set(new PendingRequestContextStackTrace(toPush)); + + if (sampler.isSampled(PendingRequestContextStackTrace.class)) { + pendingRequestCtx.set(new PendingRequestContextStackTrace(toPush, true)); + } else { + pendingRequestCtx.set(new PendingRequestContextStackTrace(toPush, false)); + } return delegate.push(toPush); } @@ -79,16 +97,14 @@ public T currentOrNull() { return delegate.currentOrNull(); } - private static class PendingRequestContextStackTrace extends IllegalStateException { + static class PendingRequestContextStackTrace extends RuntimeException { private static final long serialVersionUID = -689451606253441556L; - final RequestContext context; - - PendingRequestContextStackTrace(RequestContext context) { + PendingRequestContextStackTrace(RequestContext context, boolean isSample) { super("At thread [" + currentThread().getName() + "], previous RequestContext didn't popped : " + - context + ", It is pushed at the following stacktrace"); - this.context = context; + context + (isSample ? ", It is pushed at the following stacktrace" : ""), null, + true, isSample); } } } diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/ServiceRequestContextTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/ServiceRequestContextTest.java index 760ed5bdc70..7f57906945a 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/ServiceRequestContextTest.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/ServiceRequestContextTest.java @@ -26,6 +26,7 @@ import com.google.common.collect.ImmutableList; import com.linecorp.armeria.client.ClientRequestContext; +import com.linecorp.armeria.common.LeakTracingRequestContextStorage.PendingRequestContextStackTrace; import com.linecorp.armeria.common.annotation.Nullable; import com.linecorp.armeria.common.util.SafeCloseable; import com.linecorp.armeria.server.ServiceRequestContext; @@ -155,7 +156,7 @@ void pushWithOldIrrelevantClientCtx() { try (SafeCloseable ignored = cctx.push()) { assertCurrentCtx(cctx); final ServiceRequestContext sctx = serviceRequestContext(); - assertThatThrownBy(sctx::push).isInstanceOf(IllegalStateException.class); + assertThatThrownBy(sctx::push).isInstanceOf(PendingRequestContextStackTrace.class); } assertCurrentCtx(null); } @@ -166,7 +167,7 @@ void pushWithOldIrrelevantServiceCtx() { final ServiceRequestContext sctx2 = serviceRequestContext(); try (SafeCloseable ignored = sctx1.push()) { assertCurrentCtx(sctx1); - assertThatThrownBy(sctx2::push).isInstanceOf(IllegalStateException.class); + assertThatThrownBy(sctx2::push).isInstanceOf(PendingRequestContextStackTrace.class); } assertCurrentCtx(null); } diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java index cf2aff70268..5539cb9e42c 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java @@ -108,7 +108,7 @@ void singleTreadContextLeak() throws InterruptedException { latch.await(); assertThat(isThrown).isTrue(); - assertThat(exception.get()).hasMessageContaining("It is pushed at the following stacktrace"); + assertThat(exception.get()).hasMessageContaining("RequestContext didn't popped"); deferredClose.closeAll(); } @@ -197,7 +197,7 @@ void multiThreadContextLeak() throws InterruptedException { latch.await(); assertThat(isThrown).isTrue(); - assertThat(exception.get()).hasMessageContaining("It is pushed at the following stacktrace"); + assertThat(exception.get()).hasMessageContaining("RequestContext didn't popped"); deferredClose.closeAll(); } From c431103d9b23211e95dd312ba42a4b6b58ab8b8b Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Sun, 1 May 2022 22:04:54 +0700 Subject: [PATCH 04/44] Add benchmark on LeakTracingRequestContextStorage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Benchmark result: ``` Benchmark Mode Cnt Score Error Units LeakTracingRequestContextStorageBenchmark.baseline_threadLocal thrpt 5 138905863.970 ± 1832287.597 ops/s LeakTracingRequestContextStorageBenchmark.leakTracing_always_sample thrpt 5 1347325.018 ± 7907.388 ops/s LeakTracingRequestContextStorageBenchmark.leakTracing_never_sample thrpt 5 10762418.543 ± 43741.747 ops/s LeakTracingRequestContextStorageBenchmark.leakTracing_random_1 thrpt 5 9351586.968 ± 8154.426 ops/s LeakTracingRequestContextStorageBenchmark.leakTracing_random_10 thrpt 5 5915767.799 ± 15699.257 ops/s LeakTracingRequestContextStorageBenchmark.leakTracing_rateLimited_1 thrpt 5 9677373.483 ± 46283.720 ops/s LeakTracingRequestContextStorageBenchmark.leakTracing_rateLimited_10 thrpt 5 10027495.272 ± 129500.874 ops/s ``` --- ...TracingRequestContextStorageBenchmark.java | 91 +++++++++++++++++++ 1 file changed, 91 insertions(+) create mode 100644 benchmarks/jmh/src/jmh/java/com/linecorp/armeria/common/LeakTracingRequestContextStorageBenchmark.java diff --git a/benchmarks/jmh/src/jmh/java/com/linecorp/armeria/common/LeakTracingRequestContextStorageBenchmark.java b/benchmarks/jmh/src/jmh/java/com/linecorp/armeria/common/LeakTracingRequestContextStorageBenchmark.java new file mode 100644 index 00000000000..4920dd991fc --- /dev/null +++ b/benchmarks/jmh/src/jmh/java/com/linecorp/armeria/common/LeakTracingRequestContextStorageBenchmark.java @@ -0,0 +1,91 @@ +/* + * Copyright 2022 LINE Corporation + * + * LINE Corporation licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package com.linecorp.armeria.common; + +import org.openjdk.jmh.annotations.Benchmark; + +import com.linecorp.armeria.common.util.Sampler; +import com.linecorp.armeria.server.ServiceRequestContext; + +/** + * Microbenchmarks for LeakTracingRequestContextStorage. + */ +public class LeakTracingRequestContextStorageBenchmark { + + private static final RequestContextStorage threadLocalReqCtxStorage = + RequestContextStorage.threadLocal(); + private static final RequestContextStorage neverSample = + new LeakTracingRequestContextStorage(threadLocalReqCtxStorage, Sampler.never()); + private static final RequestContextStorage rateLimited1 = + new LeakTracingRequestContextStorage(threadLocalReqCtxStorage, Sampler.of("rate-limited=1")); + private static final RequestContextStorage rateLimited10 = + new LeakTracingRequestContextStorage(threadLocalReqCtxStorage, Sampler.of("rate-limited=10")); + private static final RequestContextStorage random1 = + new LeakTracingRequestContextStorage(threadLocalReqCtxStorage, Sampler.of("random=0.01")); + private static final RequestContextStorage random10 = + new LeakTracingRequestContextStorage(threadLocalReqCtxStorage, Sampler.of("random=0.10")); + private static final RequestContextStorage alwaysSample = + new LeakTracingRequestContextStorage(threadLocalReqCtxStorage, Sampler.always()); + private static final RequestContext reqCtx = newCtx("/"); + + private static ServiceRequestContext newCtx(String path) { + return ServiceRequestContext.builder(HttpRequest.of(HttpMethod.GET, path)) + .build(); + } + + @Benchmark + public void baseline_threadLocal() { + final RequestContext oldCtx = threadLocalReqCtxStorage.push(reqCtx); + threadLocalReqCtxStorage.pop(reqCtx, oldCtx); + } + + @Benchmark + public void leakTracing_never_sample() { + final RequestContext oldCtx = neverSample.push(reqCtx); + neverSample.pop(reqCtx, oldCtx); + } + + @Benchmark + public void leakTracing_rateLimited_1() { + final RequestContext oldCtx = rateLimited1.push(reqCtx); + rateLimited1.pop(reqCtx, oldCtx); + } + + @Benchmark + public void leakTracing_rateLimited_10() { + final RequestContext oldCtx = rateLimited10.push(reqCtx); + rateLimited10.pop(reqCtx, oldCtx); + } + + @Benchmark + public void leakTracing_random_1() { + final RequestContext oldCtx = random1.push(reqCtx); + random1.pop(reqCtx, oldCtx); + } + + @Benchmark + public void leakTracing_random_10() { + final RequestContext oldCtx = random10.push(reqCtx); + random10.pop(reqCtx, oldCtx); + } + + @Benchmark + public void leakTracing_always_sample() { + final RequestContext oldCtx = alwaysSample.push(reqCtx); + alwaysSample.pop(reqCtx, oldCtx); + } +} From 06c728ef2ddd85c9b5f06ad10ab429b5b6824c79 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Thu, 5 May 2022 22:36:52 +0700 Subject: [PATCH 05/44] Add finally when pop --- .../armeria/common/LeakTracingRequestContextStorage.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java b/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java index 0b1a8f7508e..71ad7ce3bed 100644 --- a/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java +++ b/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java @@ -87,8 +87,11 @@ public T push(RequestContext toPush) { @Override public void pop(RequestContext current, @Nullable RequestContext toRestore) { - delegate.pop(current, toRestore); - pendingRequestCtx.remove(); + try { + delegate.pop(current, toRestore); + } finally { + pendingRequestCtx.remove(); + } } @Nullable From 0f5e5e5a409b63f88b95fcd9c4f825dbef6cc774 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Thu, 5 May 2022 23:11:32 +0700 Subject: [PATCH 06/44] Change generic of Sampler --- .../armeria/common/LeakTracingRequestContextStorage.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java b/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java index 71ad7ce3bed..dc76d92710d 100644 --- a/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java +++ b/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java @@ -34,7 +34,7 @@ public final class LeakTracingRequestContextStorage implements RequestContextSto private final RequestContextStorage delegate; private final FastThreadLocal pendingRequestCtx; - private final Sampler> sampler; + private final Sampler sampler; /** * Creates a new instance. @@ -50,10 +50,10 @@ public LeakTracingRequestContextStorage(RequestContextStorage delegate) { * @param sampler the {@link Sampler} that determines whether to retain the stacktrace of the context leaks */ public LeakTracingRequestContextStorage(RequestContextStorage delegate, - Sampler> sampler) { + Sampler sampler) { this.delegate = delegate; pendingRequestCtx = new FastThreadLocal<>(); - this.sampler = sampler; + this.sampler = (Sampler) sampler; } @Nullable From 82bb1a83500c877c788bf7f31893a847bdb8562b Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Fri, 13 May 2022 17:14:46 +0700 Subject: [PATCH 07/44] Add UnstableApi and check null input --- .../armeria/common/LeakTracingRequestContextStorage.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java b/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java index dc76d92710d..d6ff4b2e379 100644 --- a/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java +++ b/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java @@ -21,6 +21,7 @@ import com.linecorp.armeria.client.ClientRequestContext; import com.linecorp.armeria.common.annotation.Nullable; +import com.linecorp.armeria.common.annotation.UnstableApi; import com.linecorp.armeria.common.util.Sampler; import com.linecorp.armeria.server.ServiceRequestContext; @@ -30,6 +31,7 @@ * A {@link RequestContextStorage} which keeps track of {@link RequestContext}s, reporting pushed thread * information if a {@link RequestContext} is leaked. */ +@UnstableApi public final class LeakTracingRequestContextStorage implements RequestContextStorage { private final RequestContextStorage delegate; @@ -51,9 +53,9 @@ public LeakTracingRequestContextStorage(RequestContextStorage delegate) { */ public LeakTracingRequestContextStorage(RequestContextStorage delegate, Sampler sampler) { - this.delegate = delegate; + this.delegate = requireNonNull(delegate, "delegate"); pendingRequestCtx = new FastThreadLocal<>(); - this.sampler = (Sampler) sampler; + this.sampler = (Sampler) requireNonNull(sampler, "sampler"); } @Nullable From a7fd5634b4035b91774af005b0e3f244fc95a13c Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Fri, 13 May 2022 22:49:54 +0700 Subject: [PATCH 08/44] Refactor DeferredClose --- .../common/TraceRequestContextLeakTest.java | 181 +++++++++--------- 1 file changed, 89 insertions(+), 92 deletions(-) diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java index 5539cb9e42c..f74593aa378 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java @@ -18,7 +18,6 @@ import static org.assertj.core.api.Assertions.assertThat; -import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; import java.util.concurrent.Executor; @@ -83,34 +82,33 @@ void singleTreadContextLeak() throws InterruptedException { final AtomicBoolean isThrown = new AtomicBoolean(); final AtomicReference exception = new AtomicReference<>(); final CountDownLatch latch = new CountDownLatch(2); - final DeferredClose deferredClose = new DeferredClose(); - final EventLoop executor = eventLoopExtension.get(); + try (DeferredClose deferredClose = new DeferredClose()) { + final EventLoop executor = eventLoopExtension.get(); - executor.execute(() -> { - final ServiceRequestContext ctx = newCtx("/1"); - final SafeCloseable leaked = ctx.push(); - latch.countDown(); - deferredClose.add(executor, leaked); - }); - - executor.execute(() -> { - final ServiceRequestContext anotherCtx = newCtx("/2"); - try (SafeCloseable ignore = anotherCtx.push()) { - //ignore - } catch (Exception ex) { - isThrown.set(true); - exception.set(ex); - } finally { + executor.execute(() -> { + final ServiceRequestContext ctx = newCtx("/1"); + final SafeCloseable leaked = ctx.push(); latch.countDown(); - } - }); - - latch.await(); - assertThat(isThrown).isTrue(); - assertThat(exception.get()).hasMessageContaining("RequestContext didn't popped"); - - deferredClose.closeAll(); + deferredClose.add(executor, leaked); + }); + + executor.execute(() -> { + final ServiceRequestContext anotherCtx = newCtx("/2"); + try (SafeCloseable ignore = anotherCtx.push()) { + //ignore + } catch (Exception ex) { + isThrown.set(true); + exception.set(ex); + } finally { + latch.countDown(); + } + }); + + latch.await(); + assertThat(isThrown).isTrue(); + assertThat(exception.get()).hasMessageContaining("RequestContext didn't popped"); + } } @Test @@ -118,34 +116,33 @@ void singleTreadContextLeak() throws InterruptedException { void multiThreadContextNotLeak() throws InterruptedException { final AtomicBoolean isThrown = new AtomicBoolean(false); final CountDownLatch latch = new CountDownLatch(2); - final DeferredClose deferredClose = new DeferredClose(); final EventLoopGroup executor = eventLoopGroupExtension.get(); - executor.next().execute(() -> { - final ServiceRequestContext ctx = newCtx("/1"); - final SafeCloseable leaked = ctx.push(); - latch.countDown(); - deferredClose.add(executor, leaked); - }); - - executor.next().execute(() -> { - //Leak happened on the first eventLoop doesn't make eventLoop when trying to push - final ServiceRequestContext anotherCtx = newCtx("/2"); - try (SafeCloseable ignore = anotherCtx.push()) { - //ignore - } catch (Exception ex) { - //Not suppose to throw exception on the second event loop - isThrown.set(true); - } finally { + try (DeferredClose deferredClose = new DeferredClose()) { + executor.next().execute(() -> { + final ServiceRequestContext ctx = newCtx("/1"); + final SafeCloseable leaked = ctx.push(); latch.countDown(); - } - }); - - latch.await(); - assertThat(isThrown).isFalse(); - - deferredClose.closeAll(); + deferredClose.add(executor, leaked); + }); + + executor.next().execute(() -> { + //Leak happened on the first eventLoop shouldn't affect 2nd eventLoop when trying to push + final ServiceRequestContext anotherCtx = newCtx("/2"); + try (SafeCloseable ignore = anotherCtx.push()) { + //Ignore + } catch (Exception ex) { + //Not suppose to throw exception on the second event loop + isThrown.set(true); + } finally { + latch.countDown(); + } + }); + + latch.await(); + assertThat(isThrown).isFalse(); + } } @Test @@ -154,7 +151,6 @@ void multiThreadContextLeak() throws InterruptedException { final AtomicBoolean isThrown = new AtomicBoolean(false); final AtomicReference exception = new AtomicReference<>(); final CountDownLatch latch = new CountDownLatch(3); - final DeferredClose deferredClose = new DeferredClose(); final EventLoopGroup executor = eventLoopGroupExtension.get(); @@ -164,42 +160,42 @@ void multiThreadContextLeak() throws InterruptedException { final Executor ex1 = executor.next(); final Executor ex2 = executor.next(); - ex1.execute(() -> { - final SafeCloseable leaked = ctx.push(); - latch.countDown(); - deferredClose.add(ex1, leaked); - }); - - ex2.execute(() -> { - try { - //intentional leak - final SafeCloseable leaked = anotherCtx.push(); - deferredClose.add(ex2, leaked); - } catch (Exception ex) { - isThrown.set(true); - } finally { - latch.countDown(); - } - }); - - assertThat(isThrown).isFalse(); - - ex1.execute(() -> { - try (SafeCloseable ignore = anotherCtx.push()) { - //ignore - } catch (Exception ex) { - isThrown.set(true); - exception.set(ex); - } finally { + try (DeferredClose deferredClose = new DeferredClose()) { + ex1.execute(() -> { + final SafeCloseable leaked = ctx.push(); latch.countDown(); - } - }); - - latch.await(); - assertThat(isThrown).isTrue(); - assertThat(exception.get()).hasMessageContaining("RequestContext didn't popped"); - - deferredClose.closeAll(); + deferredClose.add(ex1, leaked); + }); + + ex2.execute(() -> { + try { + //Intentional leak + final SafeCloseable leaked = anotherCtx.push(); + deferredClose.add(ex2, leaked); + } catch (Exception ex) { + isThrown.set(true); + } finally { + latch.countDown(); + } + }); + + assertThat(isThrown).isFalse(); + + ex1.execute(() -> { + try (SafeCloseable ignore = anotherCtx.push()) { + //ignore + } catch (Exception ex) { + isThrown.set(true); + exception.set(ex); + } finally { + latch.countDown(); + } + }); + + latch.await(); + assertThat(isThrown).isTrue(); + assertThat(exception.get()).hasMessageContaining("RequestContext didn't popped"); + } } private static ServiceRequestContext newCtx(String path) { @@ -208,20 +204,21 @@ private static ServiceRequestContext newCtx(String path) { } //Utility to clean up RequestContext leak after test - private static class DeferredClose { + private static class DeferredClose implements SafeCloseable { - private final AtomicReference> toClose; + private final ConcurrentHashMap toClose; DeferredClose() { - toClose = new AtomicReference<>(new ConcurrentHashMap<>()); + toClose = new ConcurrentHashMap<>(); } void add(Executor executor, SafeCloseable closeable) { - toClose.get().put(executor, closeable); + toClose.put(executor, closeable); } - void closeAll() { - toClose.get().forEach((executor, closeable) -> executor.execute(closeable::close)); + @Override + public void close() { + toClose.forEach((executor, closeable) -> executor.execute(closeable::close)); } } } From ea2e675b53976e82b187544d65886134c54f900e Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Sat, 14 May 2022 15:08:35 +0700 Subject: [PATCH 09/44] Add current stacktrace to exception --- .../LeakTracingRequestContextStorage.java | 5 +++-- .../internal/common/RequestContextUtil.java | 18 ++++++++++++++---- .../common/TraceRequestContextLeakTest.java | 4 ++-- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java b/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java index d6ff4b2e379..ef111b4d097 100644 --- a/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java +++ b/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java @@ -16,6 +16,7 @@ package com.linecorp.armeria.common; +import static com.linecorp.armeria.internal.common.RequestContextUtil.newIllegalContextPushingException; import static java.lang.Thread.currentThread; import static java.util.Objects.requireNonNull; @@ -72,9 +73,9 @@ public T push(RequestContext toPush) { // The delegate has the ServiceRequestContext whose root() is toPush } else if (toPush instanceof ClientRequestContext && prevContext.root() == toPush.root()) { - // The delegate has the ClientRequestContext whose root() is the same toPush.root() + // The delegate has the ClientRequestContext whose root() is the same as toPush.root() } else { - throw pendingRequestCtx.get(); + throw newIllegalContextPushingException(prevContext, toPush, pendingRequestCtx.get()); } } diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java index c44bae563d6..f3936d95312 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java @@ -89,12 +89,22 @@ public static SafeCloseable noopSafeCloseable() { */ public static IllegalStateException newIllegalContextPushingException( RequestContext newCtx, RequestContext oldCtx) { + return newIllegalContextPushingException(newCtx, oldCtx, null); + } + + /** + * Returns an {@link IllegalStateException} which is raised when pushing a context from + * the unexpected thread or forgetting to close the previous context. + */ + public static IllegalStateException newIllegalContextPushingException( + RequestContext newCtx, RequestContext oldCtx, @Nullable Throwable cause) { requireNonNull(newCtx, "newCtx"); requireNonNull(oldCtx, "oldCtx"); - final IllegalStateException ex = new IllegalStateException( - "Trying to call object wrapped with context " + newCtx + ", but context is currently " + - "set to " + oldCtx + ". This means the callback was called from " + - "unexpected thread or forgetting to close previous context."); + final String message = "Trying to call object wrapped with context " + newCtx + ", but context is currently " + + "set to " + oldCtx + ". This means the callback was called from " + + "unexpected thread or forgetting to close previous context."; + final IllegalStateException ex = cause == null ? new IllegalStateException(message) + : new IllegalStateException(message, cause); if (REPORTED_THREADS.add(Thread.currentThread())) { logger.warn("An error occurred while pushing a context", ex); } diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java index f74593aa378..5eb2b1ab045 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java @@ -107,7 +107,7 @@ void singleTreadContextLeak() throws InterruptedException { latch.await(); assertThat(isThrown).isTrue(); - assertThat(exception.get()).hasMessageContaining("RequestContext didn't popped"); + assertThat(exception.get()).getRootCause().hasMessageContaining("RequestContext didn't popped"); } } @@ -194,7 +194,7 @@ void multiThreadContextLeak() throws InterruptedException { latch.await(); assertThat(isThrown).isTrue(); - assertThat(exception.get()).hasMessageContaining("RequestContext didn't popped"); + assertThat(exception.get()).getRootCause().hasMessageContaining("RequestContext didn't popped"); } } From cb8dcf425c9f2ba08875213016e5a5d6910152bf Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Sat, 14 May 2022 15:36:32 +0700 Subject: [PATCH 10/44] Use await instead of CountDownLatch in some tests --- .../common/TraceRequestContextLeakTest.java | 27 ++++++------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java index 5eb2b1ab045..1fdf6ad9b1d 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java @@ -17,6 +17,7 @@ package com.linecorp.armeria.common; import static org.assertj.core.api.Assertions.assertThat; +import static org.awaitility.Awaitility.await; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; @@ -53,7 +54,7 @@ void singleThreadContextNotLeak() throws InterruptedException { executor.execute(() -> { final ServiceRequestContext ctx = newCtx("/1"); try (SafeCloseable ignore = ctx.push()) { - //ignore + //Ignore } catch (Exception ex) { isThrown.set(true); } finally { @@ -64,7 +65,7 @@ void singleThreadContextNotLeak() throws InterruptedException { executor.execute(() -> { final ServiceRequestContext anotherCtx = newCtx("/2"); try (SafeCloseable ignore = anotherCtx.push()) { - //ignore + //Ignore } catch (Exception ex) { isThrown.set(true); } finally { @@ -81,7 +82,6 @@ void singleThreadContextNotLeak() throws InterruptedException { void singleTreadContextLeak() throws InterruptedException { final AtomicBoolean isThrown = new AtomicBoolean(); final AtomicReference exception = new AtomicReference<>(); - final CountDownLatch latch = new CountDownLatch(2); try (DeferredClose deferredClose = new DeferredClose()) { final EventLoop executor = eventLoopExtension.get(); @@ -89,31 +89,27 @@ void singleTreadContextLeak() throws InterruptedException { executor.execute(() -> { final ServiceRequestContext ctx = newCtx("/1"); final SafeCloseable leaked = ctx.push(); - latch.countDown(); deferredClose.add(executor, leaked); }); executor.execute(() -> { final ServiceRequestContext anotherCtx = newCtx("/2"); try (SafeCloseable ignore = anotherCtx.push()) { - //ignore + //Ignore } catch (Exception ex) { isThrown.set(true); exception.set(ex); - } finally { - latch.countDown(); } }); - latch.await(); - assertThat(isThrown).isTrue(); + await().untilTrue(isThrown); assertThat(exception.get()).getRootCause().hasMessageContaining("RequestContext didn't popped"); } } @Test @SuppressWarnings("MustBeClosedChecker") - void multiThreadContextNotLeak() throws InterruptedException { + void multiThreadContextLeakNotInterfereOthersEventLoop() throws InterruptedException { final AtomicBoolean isThrown = new AtomicBoolean(false); final CountDownLatch latch = new CountDownLatch(2); @@ -150,7 +146,6 @@ void multiThreadContextNotLeak() throws InterruptedException { void multiThreadContextLeak() throws InterruptedException { final AtomicBoolean isThrown = new AtomicBoolean(false); final AtomicReference exception = new AtomicReference<>(); - final CountDownLatch latch = new CountDownLatch(3); final EventLoopGroup executor = eventLoopGroupExtension.get(); @@ -163,7 +158,6 @@ void multiThreadContextLeak() throws InterruptedException { try (DeferredClose deferredClose = new DeferredClose()) { ex1.execute(() -> { final SafeCloseable leaked = ctx.push(); - latch.countDown(); deferredClose.add(ex1, leaked); }); @@ -174,8 +168,6 @@ void multiThreadContextLeak() throws InterruptedException { deferredClose.add(ex2, leaked); } catch (Exception ex) { isThrown.set(true); - } finally { - latch.countDown(); } }); @@ -183,17 +175,14 @@ void multiThreadContextLeak() throws InterruptedException { ex1.execute(() -> { try (SafeCloseable ignore = anotherCtx.push()) { - //ignore + //Ignore } catch (Exception ex) { isThrown.set(true); exception.set(ex); - } finally { - latch.countDown(); } }); - latch.await(); - assertThat(isThrown).isTrue(); + await().untilTrue(isThrown); assertThat(exception.get()).getRootCause().hasMessageContaining("RequestContext didn't popped"); } } From 50d4127ab72daa6b234bac99ff4adc931efcd859 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Sat, 14 May 2022 16:10:03 +0700 Subject: [PATCH 11/44] Fix lint --- .../linecorp/armeria/internal/common/RequestContextUtil.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java index f3936d95312..5ac44baabf4 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java @@ -100,8 +100,8 @@ public static IllegalStateException newIllegalContextPushingException( RequestContext newCtx, RequestContext oldCtx, @Nullable Throwable cause) { requireNonNull(newCtx, "newCtx"); requireNonNull(oldCtx, "oldCtx"); - final String message = "Trying to call object wrapped with context " + newCtx + ", but context is currently " + - "set to " + oldCtx + ". This means the callback was called from " + + final String message = "Trying to call object wrapped with context " + newCtx + ", but context is " + + "currently set to " + oldCtx + ". This means the callback was called from " + "unexpected thread or forgetting to close previous context."; final IllegalStateException ex = cause == null ? new IllegalStateException(message) : new IllegalStateException(message, cause); From 818c166ded21f134903c489a7f0ea4b61c0ae421 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Sat, 14 May 2022 17:33:01 +0700 Subject: [PATCH 12/44] Fix test --- .../linecorp/armeria/common/ServiceRequestContextTest.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/ServiceRequestContextTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/ServiceRequestContextTest.java index 7f57906945a..760ed5bdc70 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/ServiceRequestContextTest.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/ServiceRequestContextTest.java @@ -26,7 +26,6 @@ import com.google.common.collect.ImmutableList; import com.linecorp.armeria.client.ClientRequestContext; -import com.linecorp.armeria.common.LeakTracingRequestContextStorage.PendingRequestContextStackTrace; import com.linecorp.armeria.common.annotation.Nullable; import com.linecorp.armeria.common.util.SafeCloseable; import com.linecorp.armeria.server.ServiceRequestContext; @@ -156,7 +155,7 @@ void pushWithOldIrrelevantClientCtx() { try (SafeCloseable ignored = cctx.push()) { assertCurrentCtx(cctx); final ServiceRequestContext sctx = serviceRequestContext(); - assertThatThrownBy(sctx::push).isInstanceOf(PendingRequestContextStackTrace.class); + assertThatThrownBy(sctx::push).isInstanceOf(IllegalStateException.class); } assertCurrentCtx(null); } @@ -167,7 +166,7 @@ void pushWithOldIrrelevantServiceCtx() { final ServiceRequestContext sctx2 = serviceRequestContext(); try (SafeCloseable ignored = sctx1.push()) { assertCurrentCtx(sctx1); - assertThatThrownBy(sctx2::push).isInstanceOf(PendingRequestContextStackTrace.class); + assertThatThrownBy(sctx2::push).isInstanceOf(IllegalStateException.class); } assertCurrentCtx(null); } From 5207a4ca3107017c6be9bee4f713f067785e5b79 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Tue, 24 May 2022 16:20:50 +0700 Subject: [PATCH 13/44] Provide a flag for enable requestContextLeakDetection --- .../armeria/common/DefaultFlagsProvider.java | 6 ++ .../com/linecorp/armeria/common/Flags.java | 19 ++++++ .../armeria/common/FlagsProvider.java | 17 +++++ .../common/SystemPropertyFlagsProvider.java | 21 ++++++ .../common/LeakDetectionConfiguration.java | 65 +++++++++++++++++++ .../internal/common/RequestContextUtil.java | 10 ++- .../linecorp/armeria/common/FlagsTest.java | 27 ++++++++ ... => EnableLeakDetectionFlagsProvider.java} | 14 +++- .../com.linecorp.armeria.common.FlagsProvider | 1 + ...meria.common.RequestContextStorageProvider | 1 - 10 files changed, 176 insertions(+), 5 deletions(-) create mode 100644 core/src/main/java/com/linecorp/armeria/internal/common/LeakDetectionConfiguration.java rename it/trace-context-leak/src/test/java/com/linecorp/armeria/common/{LeakTracingRequestContextStorageProvider.java => EnableLeakDetectionFlagsProvider.java} (63%) create mode 100644 it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.FlagsProvider delete mode 100644 it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.RequestContextStorageProvider diff --git a/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java index a190ab84abb..acd69ab8399 100644 --- a/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java @@ -28,6 +28,7 @@ import com.linecorp.armeria.common.util.Sampler; import com.linecorp.armeria.common.util.TransportType; +import com.linecorp.armeria.internal.common.LeakDetectionConfiguration; import com.linecorp.armeria.server.TransientServiceOption; /** @@ -398,4 +399,9 @@ public Path defaultMultipartUploadsLocation() { File.separatorChar + "armeria" + File.separatorChar + "multipart-uploads"); } + + @Override + public LeakDetectionConfiguration requestContextLeakDetection() { + return LeakDetectionConfiguration.disable(); + } } diff --git a/core/src/main/java/com/linecorp/armeria/common/Flags.java b/core/src/main/java/com/linecorp/armeria/common/Flags.java index f5f0a50a618..ec63b7860ef 100644 --- a/core/src/main/java/com/linecorp/armeria/common/Flags.java +++ b/core/src/main/java/com/linecorp/armeria/common/Flags.java @@ -53,6 +53,7 @@ import com.linecorp.armeria.common.util.Sampler; import com.linecorp.armeria.common.util.SystemInfo; import com.linecorp.armeria.common.util.TransportType; +import com.linecorp.armeria.internal.common.LeakDetectionConfiguration; import com.linecorp.armeria.internal.common.util.SslContextUtil; import com.linecorp.armeria.server.HttpService; import com.linecorp.armeria.server.ServerBuilder; @@ -376,6 +377,9 @@ public final class Flags { private static final Path DEFAULT_MULTIPART_UPLOADS_LOCATION = getValue(FlagsProvider::defaultMultipartUploadsLocation, "defaultMultipartUploadsLocation"); + private static final LeakDetectionConfiguration REQUEST_CONTEXT_LEAK_DETECTION = + getValue(FlagsProvider::requestContextLeakDetection, "requestContextLeakDetection"); + /** * Returns the specification of the {@link Sampler} that determines whether to retain the stack * trace of the exceptions that are thrown frequently by Armeria. A sampled exception will have the stack @@ -1295,6 +1299,21 @@ public static boolean allowDoubleDotsInQueryString() { return ALLOW_DOUBLE_DOTS_IN_QUERY_STRING; } + /** + * Returns the {@link LeakDetectionConfiguration} that determines whether to trace the stack trace of + * request context leaks and how frequently to keeps stack trace. A sampled exception will have the stack + * trace while the others will have an empty stack trace to eliminate the cost of capturing the stack + * trace. + * + *

This flag is disabled by default. + * Specify the {@code -Dcom.linecorp.armeria.requestContextLeakDetection=} JVM option to + * override the default. By providing specification of {@link Sampler}, {@link LeakDetectionConfiguration} + * is enable. See {@link Sampler#of(String)} for the specification string format.

+ */ + public static LeakDetectionConfiguration requestContextLeakDetection() { + return REQUEST_CONTEXT_LEAK_DETECTION; + } + @Nullable private static String nullableCaffeineSpec(Function method, String flagName) { return caffeineSpec(method, flagName, true); diff --git a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java index ca6f44098e7..126b08e4144 100644 --- a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java @@ -43,6 +43,7 @@ import com.linecorp.armeria.common.util.Sampler; import com.linecorp.armeria.common.util.SystemInfo; import com.linecorp.armeria.common.util.TransportType; +import com.linecorp.armeria.internal.common.LeakDetectionConfiguration; import com.linecorp.armeria.server.HttpService; import com.linecorp.armeria.server.ServerBuilder; import com.linecorp.armeria.server.Service; @@ -960,4 +961,20 @@ default Boolean allowDoubleDotsInQueryString() { default Path defaultMultipartUploadsLocation() { return null; } + + /** + * Returns the {@link LeakDetectionConfiguration} that determines whether to trace the stack trace of + * request context leaks and how frequently to keeps stack trace. A sampled exception will have the stack + * trace while the others will have an empty stack trace to eliminate the cost of capturing the stack + * trace. + * + *

This flag is disabled by default. + * Specify the {@code -Dcom.linecorp.armeria.requestContextLeakDetection=} JVM option to + * override the default. By providing specification of {@link Sampler}, {@link LeakDetectionConfiguration} + * is enable. See {@link Sampler#of(String)} for the specification string format.

+ */ + @Nullable + default LeakDetectionConfiguration requestContextLeakDetection() { + return null; + } } diff --git a/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java index bffd83d138e..461fd41a7da 100644 --- a/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java @@ -40,6 +40,7 @@ import com.linecorp.armeria.common.util.InetAddressPredicates; import com.linecorp.armeria.common.util.Sampler; import com.linecorp.armeria.common.util.TransportType; +import com.linecorp.armeria.internal.common.LeakDetectionConfiguration; import com.linecorp.armeria.server.TransientServiceOption; /** @@ -438,6 +439,26 @@ public Path defaultMultipartUploadsLocation() { return getAndParse("defaultMultipartUploadsLocation", Paths::get); } + @Override + public LeakDetectionConfiguration requestContextLeakDetection() { + final String spec = getNormalized("requestContextLeakDetection"); + if (spec == null) { + return null; + } + if ("true".equals(spec) || "always".equals(spec)) { + return LeakDetectionConfiguration.enable(Sampler.always()); + } + if ("false".equals(spec) || "never".equals(spec)) { + return LeakDetectionConfiguration.enable(Sampler.never()); + } + try { + return LeakDetectionConfiguration.enable(Sampler.of(spec)); + } catch (Exception e) { + // Invalid sampler specification + throw new IllegalArgumentException("invalid sampler spec: " + spec, e); + } + } + @Nullable private static Long getLong(String name) { return getAndParse(name, Long::parseLong); diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/LeakDetectionConfiguration.java b/core/src/main/java/com/linecorp/armeria/internal/common/LeakDetectionConfiguration.java new file mode 100644 index 00000000000..32285b228a0 --- /dev/null +++ b/core/src/main/java/com/linecorp/armeria/internal/common/LeakDetectionConfiguration.java @@ -0,0 +1,65 @@ +/* + * Copyright 2022 LINE Corporation + * + * LINE Corporation licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package com.linecorp.armeria.internal.common; + +import static java.util.Objects.requireNonNull; + +import com.google.common.base.MoreObjects; + +import com.linecorp.armeria.common.util.Sampler; + +/** + * The configuration that determines whether to trace request context leaks and how frequently to + * keeps stack trace + */ +public final class LeakDetectionConfiguration { + + private static final LeakDetectionConfiguration DISABLE_INSTANCE = + new LeakDetectionConfiguration(false, null); + + public static LeakDetectionConfiguration disable() { + return DISABLE_INSTANCE; + } + + public static LeakDetectionConfiguration enable(Sampler sampler) { + return new LeakDetectionConfiguration(true, requireNonNull(sampler, "sampler")); + } + + private final Boolean isEnable; + private final Sampler sampler; + + LeakDetectionConfiguration(boolean isEnable, Sampler sampler) { + this.isEnable = isEnable; + this.sampler = sampler; + } + + public boolean isEnable() { + return isEnable; + } + + public Sampler sampler() { + return sampler; + } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("isEnable", isEnable) + .add("sampler", sampler) + .toString(); + } +} diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java index 5ac44baabf4..a866ec2b41e 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java @@ -33,6 +33,7 @@ import com.linecorp.armeria.client.DefaultClientRequestContext; import com.linecorp.armeria.common.Flags; import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.LeakTracingRequestContextStorage; import com.linecorp.armeria.common.RequestContext; import com.linecorp.armeria.common.RequestContextStorage; import com.linecorp.armeria.common.RequestContextStorageProvider; @@ -63,8 +64,15 @@ public final class RequestContextUtil { static { final RequestContextStorageProvider provider = Flags.requestContextStorageProvider(); + final LeakDetectionConfiguration leakDetectionConfiguration = Flags.requestContextLeakDetection(); try { - requestContextStorage = provider.newStorage(); + if (leakDetectionConfiguration.isEnable()) { + requestContextStorage = + new LeakTracingRequestContextStorage(provider.newStorage(), + leakDetectionConfiguration.sampler()); + } else { + requestContextStorage = provider.newStorage(); + } } catch (Throwable t) { throw new IllegalStateException("Failed to create context storage. provider: " + provider, t); } diff --git a/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java b/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java index 35db9925443..84dbb277adc 100644 --- a/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java +++ b/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java @@ -48,6 +48,7 @@ import com.linecorp.armeria.common.util.Exceptions; import com.linecorp.armeria.common.util.Sampler; import com.linecorp.armeria.common.util.TransportType; +import com.linecorp.armeria.internal.common.LeakDetectionConfiguration; import com.linecorp.armeria.server.TransientServiceOption; import io.netty.channel.epoll.Epoll; @@ -222,6 +223,32 @@ void invalidSystemPropertyVerboseExceptionSampler() throws Throwable { .isEqualTo(new ExceptionSampler("rate-limit=10")); } + @Test + void defaultRequestContextLeakDetection() throws Exception { + final Method method = flags.getDeclaredMethod("requestContextLeakDetection"); + assertThat(method.invoke(flags)) + .usingRecursiveComparison() + .isEqualTo(LeakDetectionConfiguration.disable()); + } + + @Test + @SetSystemProperty(key = "com.linecorp.armeria.requestContextLeakDetection", value = "always") + void systemPropertyRequestContextLeakDetection() throws Exception { + final Method method = flags.getDeclaredMethod("requestContextLeakDetection"); + assertThat(method.invoke(flags)) + .usingRecursiveComparison() + .isEqualTo(LeakDetectionConfiguration.enable(Sampler.always())); + } + + @Test + @SetSystemProperty(key = "com.linecorp.armeria.requestContextLeakDetection", value = "invalid-spec") + void invalidSystemPropertyRequestContextLeakDetection() throws Exception { + final Method method = flags.getDeclaredMethod("requestContextLeakDetection"); + assertThat(method.invoke(flags)) + .usingRecursiveComparison() + .isEqualTo(LeakDetectionConfiguration.disable()); + } + @Test void testApiConsistencyBetweenFlagsAndFlagsProvider() { //Check method consistency between Flags and FlagsProvider excluding deprecated methods diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/LeakTracingRequestContextStorageProvider.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/EnableLeakDetectionFlagsProvider.java similarity index 63% rename from it/trace-context-leak/src/test/java/com/linecorp/armeria/common/LeakTracingRequestContextStorageProvider.java rename to it/trace-context-leak/src/test/java/com/linecorp/armeria/common/EnableLeakDetectionFlagsProvider.java index 6257976b45c..553dc0de337 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/LeakTracingRequestContextStorageProvider.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/EnableLeakDetectionFlagsProvider.java @@ -16,10 +16,18 @@ package com.linecorp.armeria.common; -public final class LeakTracingRequestContextStorageProvider implements RequestContextStorageProvider { +import com.linecorp.armeria.common.util.Sampler; +import com.linecorp.armeria.internal.common.LeakDetectionConfiguration; + +public final class EnableLeakDetectionFlagsProvider implements FlagsProvider { + + @Override + public int priority() { + return 10; + } @Override - public RequestContextStorage newStorage() { - return new LeakTracingRequestContextStorage(RequestContextStorage.threadLocal()); + public LeakDetectionConfiguration requestContextLeakDetection() { + return LeakDetectionConfiguration.enable(Sampler.always()); } } diff --git a/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.FlagsProvider b/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.FlagsProvider new file mode 100644 index 00000000000..a837ca191b5 --- /dev/null +++ b/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.FlagsProvider @@ -0,0 +1 @@ +com.linecorp.armeria.common.EnableLeakDetectionFlagsProvider \ No newline at end of file diff --git a/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.RequestContextStorageProvider b/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.RequestContextStorageProvider deleted file mode 100644 index 52903537ec6..00000000000 --- a/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.RequestContextStorageProvider +++ /dev/null @@ -1 +0,0 @@ -com.linecorp.armeria.common.LeakTracingRequestContextStorageProvider From d24e0a261d5e5a06435ef770b209b31c49d5e51b Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Tue, 24 May 2022 22:31:16 +0700 Subject: [PATCH 14/44] Move LeakDetectionConfiguration package outside of internal --- .../armeria/common/DefaultFlagsProvider.java | 1 - .../java/com/linecorp/armeria/common/Flags.java | 1 - .../linecorp/armeria/common/FlagsProvider.java | 1 - .../common/LeakDetectionConfiguration.java | 16 ++++++++++++++-- .../common/SystemPropertyFlagsProvider.java | 1 - .../internal/common/RequestContextUtil.java | 1 + .../com/linecorp/armeria/common/FlagsTest.java | 1 - .../common/EnableLeakDetectionFlagsProvider.java | 1 - 8 files changed, 15 insertions(+), 8 deletions(-) rename core/src/main/java/com/linecorp/armeria/{internal => }/common/LeakDetectionConfiguration.java (79%) diff --git a/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java index acd69ab8399..1194e1927d0 100644 --- a/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java @@ -28,7 +28,6 @@ import com.linecorp.armeria.common.util.Sampler; import com.linecorp.armeria.common.util.TransportType; -import com.linecorp.armeria.internal.common.LeakDetectionConfiguration; import com.linecorp.armeria.server.TransientServiceOption; /** diff --git a/core/src/main/java/com/linecorp/armeria/common/Flags.java b/core/src/main/java/com/linecorp/armeria/common/Flags.java index ec63b7860ef..ffe512cc054 100644 --- a/core/src/main/java/com/linecorp/armeria/common/Flags.java +++ b/core/src/main/java/com/linecorp/armeria/common/Flags.java @@ -53,7 +53,6 @@ import com.linecorp.armeria.common.util.Sampler; import com.linecorp.armeria.common.util.SystemInfo; import com.linecorp.armeria.common.util.TransportType; -import com.linecorp.armeria.internal.common.LeakDetectionConfiguration; import com.linecorp.armeria.internal.common.util.SslContextUtil; import com.linecorp.armeria.server.HttpService; import com.linecorp.armeria.server.ServerBuilder; diff --git a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java index 126b08e4144..4098eeef7ce 100644 --- a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java @@ -43,7 +43,6 @@ import com.linecorp.armeria.common.util.Sampler; import com.linecorp.armeria.common.util.SystemInfo; import com.linecorp.armeria.common.util.TransportType; -import com.linecorp.armeria.internal.common.LeakDetectionConfiguration; import com.linecorp.armeria.server.HttpService; import com.linecorp.armeria.server.ServerBuilder; import com.linecorp.armeria.server.Service; diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/LeakDetectionConfiguration.java b/core/src/main/java/com/linecorp/armeria/common/LeakDetectionConfiguration.java similarity index 79% rename from core/src/main/java/com/linecorp/armeria/internal/common/LeakDetectionConfiguration.java rename to core/src/main/java/com/linecorp/armeria/common/LeakDetectionConfiguration.java index 32285b228a0..fb7cba60b38 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/LeakDetectionConfiguration.java +++ b/core/src/main/java/com/linecorp/armeria/common/LeakDetectionConfiguration.java @@ -14,7 +14,7 @@ * under the License. */ -package com.linecorp.armeria.internal.common; +package com.linecorp.armeria.common; import static java.util.Objects.requireNonNull; @@ -24,17 +24,23 @@ /** * The configuration that determines whether to trace request context leaks and how frequently to - * keeps stack trace + * keeps stack trace. */ public final class LeakDetectionConfiguration { private static final LeakDetectionConfiguration DISABLE_INSTANCE = new LeakDetectionConfiguration(false, null); + /** + * Return the instance of disabled {@link LeakDetectionConfiguration}. + */ public static LeakDetectionConfiguration disable() { return DISABLE_INSTANCE; } + /** + * Return the instance of enabled {@link LeakDetectionConfiguration} with specified {@link Sampler}. + */ public static LeakDetectionConfiguration enable(Sampler sampler) { return new LeakDetectionConfiguration(true, requireNonNull(sampler, "sampler")); } @@ -47,10 +53,16 @@ public static LeakDetectionConfiguration enable(Sampler sampler) { this.sampler = sampler; } + /** + * Return the whether {@link LeakDetectionConfiguration} is enabled. + */ public boolean isEnable() { return isEnable; } + /** + * Return the {@link Sampler}. If this {@link LeakDetectionConfiguration} is disabled then return null. + */ public Sampler sampler() { return sampler; } diff --git a/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java index 461fd41a7da..8805b50f1a4 100644 --- a/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java @@ -40,7 +40,6 @@ import com.linecorp.armeria.common.util.InetAddressPredicates; import com.linecorp.armeria.common.util.Sampler; import com.linecorp.armeria.common.util.TransportType; -import com.linecorp.armeria.internal.common.LeakDetectionConfiguration; import com.linecorp.armeria.server.TransientServiceOption; /** diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java index a866ec2b41e..31f50b06994 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java @@ -33,6 +33,7 @@ import com.linecorp.armeria.client.DefaultClientRequestContext; import com.linecorp.armeria.common.Flags; import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.LeakDetectionConfiguration; import com.linecorp.armeria.common.LeakTracingRequestContextStorage; import com.linecorp.armeria.common.RequestContext; import com.linecorp.armeria.common.RequestContextStorage; diff --git a/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java b/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java index 84dbb277adc..10f76a91117 100644 --- a/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java +++ b/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java @@ -48,7 +48,6 @@ import com.linecorp.armeria.common.util.Exceptions; import com.linecorp.armeria.common.util.Sampler; import com.linecorp.armeria.common.util.TransportType; -import com.linecorp.armeria.internal.common.LeakDetectionConfiguration; import com.linecorp.armeria.server.TransientServiceOption; import io.netty.channel.epoll.Epoll; diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/EnableLeakDetectionFlagsProvider.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/EnableLeakDetectionFlagsProvider.java index 553dc0de337..cedc3594d63 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/EnableLeakDetectionFlagsProvider.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/EnableLeakDetectionFlagsProvider.java @@ -17,7 +17,6 @@ package com.linecorp.armeria.common; import com.linecorp.armeria.common.util.Sampler; -import com.linecorp.armeria.internal.common.LeakDetectionConfiguration; public final class EnableLeakDetectionFlagsProvider implements FlagsProvider { From 61e767d78398d5872a4463e98d59ed02b6f4a455 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Thu, 2 Jun 2022 00:13:58 +0700 Subject: [PATCH 15/44] Replace LeakDetectionConfiguration with Sampler --- .../armeria/common/DefaultFlagsProvider.java | 4 +- .../com/linecorp/armeria/common/Flags.java | 17 ++-- .../armeria/common/FlagsProvider.java | 15 ++-- .../common/LeakDetectionConfiguration.java | 77 ------------------- .../common/SystemPropertyFlagsProvider.java | 8 +- .../internal/common/RequestContextUtil.java | 8 +- .../linecorp/armeria/common/FlagsTest.java | 6 +- .../EnableLeakDetectionFlagsProvider.java | 4 +- 8 files changed, 30 insertions(+), 109 deletions(-) delete mode 100644 core/src/main/java/com/linecorp/armeria/common/LeakDetectionConfiguration.java diff --git a/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java index 1194e1927d0..8f81914d1bd 100644 --- a/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java @@ -400,7 +400,7 @@ public Path defaultMultipartUploadsLocation() { } @Override - public LeakDetectionConfiguration requestContextLeakDetection() { - return LeakDetectionConfiguration.disable(); + public Sampler requestContextLeakDetection() { + return Sampler.never(); } } diff --git a/core/src/main/java/com/linecorp/armeria/common/Flags.java b/core/src/main/java/com/linecorp/armeria/common/Flags.java index ffe512cc054..f1ae886ffad 100644 --- a/core/src/main/java/com/linecorp/armeria/common/Flags.java +++ b/core/src/main/java/com/linecorp/armeria/common/Flags.java @@ -376,7 +376,7 @@ public final class Flags { private static final Path DEFAULT_MULTIPART_UPLOADS_LOCATION = getValue(FlagsProvider::defaultMultipartUploadsLocation, "defaultMultipartUploadsLocation"); - private static final LeakDetectionConfiguration REQUEST_CONTEXT_LEAK_DETECTION = + private static final Sampler REQUEST_CONTEXT_LEAK_DETECTION = getValue(FlagsProvider::requestContextLeakDetection, "requestContextLeakDetection"); /** @@ -1299,17 +1299,16 @@ public static boolean allowDoubleDotsInQueryString() { } /** - * Returns the {@link LeakDetectionConfiguration} that determines whether to trace the stack trace of - * request context leaks and how frequently to keeps stack trace. A sampled exception will have the stack - * trace while the others will have an empty stack trace to eliminate the cost of capturing the stack - * trace. + * Returns the {@link Sampler} that determines whether to trace the stack trace of request contexts leaks + * and how frequently to keeps stack trace. A sampled exception will have the stack trace while the others + * will have an empty stack trace to eliminate the cost of capturing the stack trace. * - *

This flag is disabled by default. + *

The default value of this flag is {@link Sampler#never()}. * Specify the {@code -Dcom.linecorp.armeria.requestContextLeakDetection=} JVM option to - * override the default. By providing specification of {@link Sampler}, {@link LeakDetectionConfiguration} - * is enable. See {@link Sampler#of(String)} for the specification string format.

+ * override the default. This feature is disable if users provide {@link Sampler#never()}. + * See {@link Sampler#of(String)} for the specification string format.

*/ - public static LeakDetectionConfiguration requestContextLeakDetection() { + public static Sampler requestContextLeakDetection() { return REQUEST_CONTEXT_LEAK_DETECTION; } diff --git a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java index 4098eeef7ce..ddbd5517b4d 100644 --- a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java @@ -962,18 +962,17 @@ default Path defaultMultipartUploadsLocation() { } /** - * Returns the {@link LeakDetectionConfiguration} that determines whether to trace the stack trace of - * request context leaks and how frequently to keeps stack trace. A sampled exception will have the stack - * trace while the others will have an empty stack trace to eliminate the cost of capturing the stack - * trace. + * Returns the {@link Sampler} that determines whether to trace the stack trace of request contexts leaks + * and how frequently to keeps stack trace. A sampled exception will have the stack trace while the others + * will have an empty stack trace to eliminate the cost of capturing the stack trace. * - *

This flag is disabled by default. + *

The default value of this flag is {@link Sampler#never()}. * Specify the {@code -Dcom.linecorp.armeria.requestContextLeakDetection=} JVM option to - * override the default. By providing specification of {@link Sampler}, {@link LeakDetectionConfiguration} - * is enable. See {@link Sampler#of(String)} for the specification string format.

+ * override the default. This feature is disable if users provide {@link Sampler#never()}. + * See {@link Sampler#of(String)} for the specification string format.

*/ @Nullable - default LeakDetectionConfiguration requestContextLeakDetection() { + default Sampler requestContextLeakDetection() { return null; } } diff --git a/core/src/main/java/com/linecorp/armeria/common/LeakDetectionConfiguration.java b/core/src/main/java/com/linecorp/armeria/common/LeakDetectionConfiguration.java deleted file mode 100644 index fb7cba60b38..00000000000 --- a/core/src/main/java/com/linecorp/armeria/common/LeakDetectionConfiguration.java +++ /dev/null @@ -1,77 +0,0 @@ -/* - * Copyright 2022 LINE Corporation - * - * LINE Corporation licenses this file to you under the Apache License, - * version 2.0 (the "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at: - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT - * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the - * License for the specific language governing permissions and limitations - * under the License. - */ - -package com.linecorp.armeria.common; - -import static java.util.Objects.requireNonNull; - -import com.google.common.base.MoreObjects; - -import com.linecorp.armeria.common.util.Sampler; - -/** - * The configuration that determines whether to trace request context leaks and how frequently to - * keeps stack trace. - */ -public final class LeakDetectionConfiguration { - - private static final LeakDetectionConfiguration DISABLE_INSTANCE = - new LeakDetectionConfiguration(false, null); - - /** - * Return the instance of disabled {@link LeakDetectionConfiguration}. - */ - public static LeakDetectionConfiguration disable() { - return DISABLE_INSTANCE; - } - - /** - * Return the instance of enabled {@link LeakDetectionConfiguration} with specified {@link Sampler}. - */ - public static LeakDetectionConfiguration enable(Sampler sampler) { - return new LeakDetectionConfiguration(true, requireNonNull(sampler, "sampler")); - } - - private final Boolean isEnable; - private final Sampler sampler; - - LeakDetectionConfiguration(boolean isEnable, Sampler sampler) { - this.isEnable = isEnable; - this.sampler = sampler; - } - - /** - * Return the whether {@link LeakDetectionConfiguration} is enabled. - */ - public boolean isEnable() { - return isEnable; - } - - /** - * Return the {@link Sampler}. If this {@link LeakDetectionConfiguration} is disabled then return null. - */ - public Sampler sampler() { - return sampler; - } - - @Override - public String toString() { - return MoreObjects.toStringHelper(this) - .add("isEnable", isEnable) - .add("sampler", sampler) - .toString(); - } -} diff --git a/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java index 8805b50f1a4..698e1c392dd 100644 --- a/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java @@ -439,19 +439,19 @@ public Path defaultMultipartUploadsLocation() { } @Override - public LeakDetectionConfiguration requestContextLeakDetection() { + public Sampler requestContextLeakDetection() { final String spec = getNormalized("requestContextLeakDetection"); if (spec == null) { return null; } if ("true".equals(spec) || "always".equals(spec)) { - return LeakDetectionConfiguration.enable(Sampler.always()); + return Sampler.always(); } if ("false".equals(spec) || "never".equals(spec)) { - return LeakDetectionConfiguration.enable(Sampler.never()); + return Sampler.never(); } try { - return LeakDetectionConfiguration.enable(Sampler.of(spec)); + return Sampler.of(spec); } catch (Exception e) { // Invalid sampler specification throw new IllegalArgumentException("invalid sampler spec: " + spec, e); diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java index 31f50b06994..699cf633b8f 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java @@ -33,13 +33,13 @@ import com.linecorp.armeria.client.DefaultClientRequestContext; import com.linecorp.armeria.common.Flags; import com.linecorp.armeria.common.HttpRequest; -import com.linecorp.armeria.common.LeakDetectionConfiguration; import com.linecorp.armeria.common.LeakTracingRequestContextStorage; import com.linecorp.armeria.common.RequestContext; import com.linecorp.armeria.common.RequestContextStorage; import com.linecorp.armeria.common.RequestContextStorageProvider; import com.linecorp.armeria.common.annotation.Nullable; import com.linecorp.armeria.common.util.SafeCloseable; +import com.linecorp.armeria.common.util.Sampler; import com.linecorp.armeria.server.DefaultServiceRequestContext; import io.netty.channel.ChannelFuture; @@ -65,12 +65,12 @@ public final class RequestContextUtil { static { final RequestContextStorageProvider provider = Flags.requestContextStorageProvider(); - final LeakDetectionConfiguration leakDetectionConfiguration = Flags.requestContextLeakDetection(); + final Sampler leakDetectionConfiguration = Flags.requestContextLeakDetection(); try { - if (leakDetectionConfiguration.isEnable()) { + if (!leakDetectionConfiguration.equals(Sampler.never())) { requestContextStorage = new LeakTracingRequestContextStorage(provider.newStorage(), - leakDetectionConfiguration.sampler()); + leakDetectionConfiguration); } else { requestContextStorage = provider.newStorage(); } diff --git a/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java b/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java index 10f76a91117..a393f2f73bf 100644 --- a/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java +++ b/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java @@ -227,7 +227,7 @@ void defaultRequestContextLeakDetection() throws Exception { final Method method = flags.getDeclaredMethod("requestContextLeakDetection"); assertThat(method.invoke(flags)) .usingRecursiveComparison() - .isEqualTo(LeakDetectionConfiguration.disable()); + .isEqualTo(Sampler.never()); } @Test @@ -236,7 +236,7 @@ void systemPropertyRequestContextLeakDetection() throws Exception { final Method method = flags.getDeclaredMethod("requestContextLeakDetection"); assertThat(method.invoke(flags)) .usingRecursiveComparison() - .isEqualTo(LeakDetectionConfiguration.enable(Sampler.always())); + .isEqualTo(Sampler.always()); } @Test @@ -245,7 +245,7 @@ void invalidSystemPropertyRequestContextLeakDetection() throws Exception { final Method method = flags.getDeclaredMethod("requestContextLeakDetection"); assertThat(method.invoke(flags)) .usingRecursiveComparison() - .isEqualTo(LeakDetectionConfiguration.disable()); + .isEqualTo(Sampler.never()); } @Test diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/EnableLeakDetectionFlagsProvider.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/EnableLeakDetectionFlagsProvider.java index cedc3594d63..35e69409a12 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/EnableLeakDetectionFlagsProvider.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/EnableLeakDetectionFlagsProvider.java @@ -26,7 +26,7 @@ public int priority() { } @Override - public LeakDetectionConfiguration requestContextLeakDetection() { - return LeakDetectionConfiguration.enable(Sampler.always()); + public Sampler requestContextLeakDetection() { + return Sampler.always(); } } From 449f051e7706a4d6de71b07b0f16e3eeb4e26e19 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Thu, 2 Jun 2022 00:31:57 +0700 Subject: [PATCH 16/44] Change flag name from requestContextLeakDetection to requestContextLeakDetectionSampler --- .../armeria/common/DefaultFlagsProvider.java | 2 +- .../java/com/linecorp/armeria/common/Flags.java | 8 ++++---- .../linecorp/armeria/common/FlagsProvider.java | 4 ++-- .../common/SystemPropertyFlagsProvider.java | 4 ++-- .../internal/common/RequestContextUtil.java | 6 +++--- .../com/linecorp/armeria/common/FlagsTest.java | 16 ++++++++-------- .../common/EnableLeakDetectionFlagsProvider.java | 2 +- 7 files changed, 21 insertions(+), 21 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java index 8f81914d1bd..d7a01a7069f 100644 --- a/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java @@ -400,7 +400,7 @@ public Path defaultMultipartUploadsLocation() { } @Override - public Sampler requestContextLeakDetection() { + public Sampler requestContextLeakDetectionSampler() { return Sampler.never(); } } diff --git a/core/src/main/java/com/linecorp/armeria/common/Flags.java b/core/src/main/java/com/linecorp/armeria/common/Flags.java index f1ae886ffad..78867da65be 100644 --- a/core/src/main/java/com/linecorp/armeria/common/Flags.java +++ b/core/src/main/java/com/linecorp/armeria/common/Flags.java @@ -376,8 +376,8 @@ public final class Flags { private static final Path DEFAULT_MULTIPART_UPLOADS_LOCATION = getValue(FlagsProvider::defaultMultipartUploadsLocation, "defaultMultipartUploadsLocation"); - private static final Sampler REQUEST_CONTEXT_LEAK_DETECTION = - getValue(FlagsProvider::requestContextLeakDetection, "requestContextLeakDetection"); + private static final Sampler REQUEST_CONTEXT_LEAK_DETECTION_SAMPLER = + getValue(FlagsProvider::requestContextLeakDetectionSampler, "requestContextLeakDetectionSampler"); /** * Returns the specification of the {@link Sampler} that determines whether to retain the stack @@ -1308,8 +1308,8 @@ public static boolean allowDoubleDotsInQueryString() { * override the default. This feature is disable if users provide {@link Sampler#never()}. * See {@link Sampler#of(String)} for the specification string format.

*/ - public static Sampler requestContextLeakDetection() { - return REQUEST_CONTEXT_LEAK_DETECTION; + public static Sampler requestContextLeakDetectionSampler() { + return REQUEST_CONTEXT_LEAK_DETECTION_SAMPLER; } @Nullable diff --git a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java index ddbd5517b4d..c316886db62 100644 --- a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java @@ -967,12 +967,12 @@ default Path defaultMultipartUploadsLocation() { * will have an empty stack trace to eliminate the cost of capturing the stack trace. * *

The default value of this flag is {@link Sampler#never()}. - * Specify the {@code -Dcom.linecorp.armeria.requestContextLeakDetection=} JVM option to + * Specify the {@code -Dcom.linecorp.armeria.requestContextLeakDetectionSampler=} JVM option to * override the default. This feature is disable if users provide {@link Sampler#never()}. * See {@link Sampler#of(String)} for the specification string format.

*/ @Nullable - default Sampler requestContextLeakDetection() { + default Sampler requestContextLeakDetectionSampler() { return null; } } diff --git a/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java index 698e1c392dd..f6fc20907bf 100644 --- a/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java @@ -439,8 +439,8 @@ public Path defaultMultipartUploadsLocation() { } @Override - public Sampler requestContextLeakDetection() { - final String spec = getNormalized("requestContextLeakDetection"); + public Sampler requestContextLeakDetectionSampler() { + final String spec = getNormalized("requestContextLeakDetectionSampler"); if (spec == null) { return null; } diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java index 699cf633b8f..b393de61763 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java @@ -65,12 +65,12 @@ public final class RequestContextUtil { static { final RequestContextStorageProvider provider = Flags.requestContextStorageProvider(); - final Sampler leakDetectionConfiguration = Flags.requestContextLeakDetection(); + final Sampler sampler = Flags.requestContextLeakDetectionSampler(); try { - if (!leakDetectionConfiguration.equals(Sampler.never())) { + if (!sampler.equals(Sampler.never())) { requestContextStorage = new LeakTracingRequestContextStorage(provider.newStorage(), - leakDetectionConfiguration); + sampler); } else { requestContextStorage = provider.newStorage(); } diff --git a/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java b/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java index a393f2f73bf..9aa212d3e52 100644 --- a/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java +++ b/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java @@ -223,26 +223,26 @@ void invalidSystemPropertyVerboseExceptionSampler() throws Throwable { } @Test - void defaultRequestContextLeakDetection() throws Exception { - final Method method = flags.getDeclaredMethod("requestContextLeakDetection"); + void defaultRequestContextLeakDetectionSampler() throws Exception { + final Method method = flags.getDeclaredMethod("requestContextLeakDetectionSampler"); assertThat(method.invoke(flags)) .usingRecursiveComparison() .isEqualTo(Sampler.never()); } @Test - @SetSystemProperty(key = "com.linecorp.armeria.requestContextLeakDetection", value = "always") - void systemPropertyRequestContextLeakDetection() throws Exception { - final Method method = flags.getDeclaredMethod("requestContextLeakDetection"); + @SetSystemProperty(key = "com.linecorp.armeria.requestContextLeakDetectionSampler", value = "always") + void systemPropertyRequestContextLeakDetectionSampler() throws Exception { + final Method method = flags.getDeclaredMethod("requestContextLeakDetectionSampler"); assertThat(method.invoke(flags)) .usingRecursiveComparison() .isEqualTo(Sampler.always()); } @Test - @SetSystemProperty(key = "com.linecorp.armeria.requestContextLeakDetection", value = "invalid-spec") - void invalidSystemPropertyRequestContextLeakDetection() throws Exception { - final Method method = flags.getDeclaredMethod("requestContextLeakDetection"); + @SetSystemProperty(key = "com.linecorp.armeria.requestContextLeakDetectionSampler", value = "invalid-spec") + void invalidSystemPropertyRequestContextLeakDetectionSampler() throws Exception { + final Method method = flags.getDeclaredMethod("requestContextLeakDetectionSampler"); assertThat(method.invoke(flags)) .usingRecursiveComparison() .isEqualTo(Sampler.never()); diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/EnableLeakDetectionFlagsProvider.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/EnableLeakDetectionFlagsProvider.java index 35e69409a12..52171ae954c 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/EnableLeakDetectionFlagsProvider.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/EnableLeakDetectionFlagsProvider.java @@ -26,7 +26,7 @@ public int priority() { } @Override - public Sampler requestContextLeakDetection() { + public Sampler requestContextLeakDetectionSampler() { return Sampler.always(); } } From 4dabd01393004f8f3fca0fcd411277a244726436 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Thu, 2 Jun 2022 00:53:51 +0700 Subject: [PATCH 17/44] Refactor RequestContextUtil --- .../java/com/linecorp/armeria/common/FlagsProvider.java | 4 ++-- .../armeria/internal/common/RequestContextUtil.java | 9 ++++----- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java index c316886db62..c49e719ced6 100644 --- a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java @@ -967,8 +967,8 @@ default Path defaultMultipartUploadsLocation() { * will have an empty stack trace to eliminate the cost of capturing the stack trace. * *

The default value of this flag is {@link Sampler#never()}. - * Specify the {@code -Dcom.linecorp.armeria.requestContextLeakDetectionSampler=} JVM option to - * override the default. This feature is disable if users provide {@link Sampler#never()}. + * Specify the {@code -Dcom.linecorp.armeria.requestContextLeakDetectionSampler=} JVM option + * to override the default. This feature is disable if users provide {@link Sampler#never()}. * See {@link Sampler#of(String)} for the specification string format.

*/ @Nullable diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java index b393de61763..9e74be03886 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java @@ -67,12 +67,11 @@ public final class RequestContextUtil { final RequestContextStorageProvider provider = Flags.requestContextStorageProvider(); final Sampler sampler = Flags.requestContextLeakDetectionSampler(); try { - if (!sampler.equals(Sampler.never())) { - requestContextStorage = - new LeakTracingRequestContextStorage(provider.newStorage(), - sampler); - } else { + if (sampler.equals(Sampler. never())) { requestContextStorage = provider.newStorage(); + } else { + requestContextStorage = + new LeakTracingRequestContextStorage(provider.newStorage(), sampler); } } catch (Throwable t) { throw new IllegalStateException("Failed to create context storage. provider: " + provider, t); From 8909ace4550ab4da430a4feb88808dbf6350e97d Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Thu, 2 Jun 2022 01:01:09 +0700 Subject: [PATCH 18/44] Correct Java doc grammar --- core/src/main/java/com/linecorp/armeria/common/Flags.java | 4 ++-- .../main/java/com/linecorp/armeria/common/FlagsProvider.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/Flags.java b/core/src/main/java/com/linecorp/armeria/common/Flags.java index 78867da65be..140bf9baf73 100644 --- a/core/src/main/java/com/linecorp/armeria/common/Flags.java +++ b/core/src/main/java/com/linecorp/armeria/common/Flags.java @@ -1304,8 +1304,8 @@ public static boolean allowDoubleDotsInQueryString() { * will have an empty stack trace to eliminate the cost of capturing the stack trace. * *

The default value of this flag is {@link Sampler#never()}. - * Specify the {@code -Dcom.linecorp.armeria.requestContextLeakDetection=} JVM option to - * override the default. This feature is disable if users provide {@link Sampler#never()}. + * Specify the {@code -Dcom.linecorp.armeria.requestContextLeakDetectionSampler=} JVM option + * to override the default. This feature is disabled if users provide the {@link Sampler#never()}. * See {@link Sampler#of(String)} for the specification string format.

*/ public static Sampler requestContextLeakDetectionSampler() { diff --git a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java index c49e719ced6..8c001845952 100644 --- a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java @@ -968,7 +968,7 @@ default Path defaultMultipartUploadsLocation() { * *

The default value of this flag is {@link Sampler#never()}. * Specify the {@code -Dcom.linecorp.armeria.requestContextLeakDetectionSampler=} JVM option - * to override the default. This feature is disable if users provide {@link Sampler#never()}. + * to override the default. This feature is disabled if users provide the {@link Sampler#never()}. * See {@link Sampler#of(String)} for the specification string format.

*/ @Nullable From fdd84903b244d98535cbb0265a842e19e8ff829c Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Thu, 2 Jun 2022 01:04:22 +0700 Subject: [PATCH 19/44] Add newline to EOF --- .../META-INF/services/com.linecorp.armeria.common.FlagsProvider | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.FlagsProvider b/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.FlagsProvider index a837ca191b5..124763199ff 100644 --- a/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.FlagsProvider +++ b/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.FlagsProvider @@ -1 +1 @@ -com.linecorp.armeria.common.EnableLeakDetectionFlagsProvider \ No newline at end of file +com.linecorp.armeria.common.EnableLeakDetectionFlagsProvider From 5562cfef8a5b778a667153af4b3d7f9359310279 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Fri, 3 Jun 2022 00:15:44 +0700 Subject: [PATCH 20/44] Correct comment and add UnstableApi --- core/src/main/java/com/linecorp/armeria/common/Flags.java | 4 +++- .../main/java/com/linecorp/armeria/common/FlagsProvider.java | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/Flags.java b/core/src/main/java/com/linecorp/armeria/common/Flags.java index 140bf9baf73..9a995d02188 100644 --- a/core/src/main/java/com/linecorp/armeria/common/Flags.java +++ b/core/src/main/java/com/linecorp/armeria/common/Flags.java @@ -49,6 +49,7 @@ import com.linecorp.armeria.client.retry.RetryingClient; import com.linecorp.armeria.client.retry.RetryingRpcClient; import com.linecorp.armeria.common.annotation.Nullable; +import com.linecorp.armeria.common.annotation.UnstableApi; import com.linecorp.armeria.common.util.Exceptions; import com.linecorp.armeria.common.util.Sampler; import com.linecorp.armeria.common.util.SystemInfo; @@ -1305,9 +1306,10 @@ public static boolean allowDoubleDotsInQueryString() { * *

The default value of this flag is {@link Sampler#never()}. * Specify the {@code -Dcom.linecorp.armeria.requestContextLeakDetectionSampler=} JVM option - * to override the default. This feature is disabled if users provide the {@link Sampler#never()}. + * to override the default. This feature is disabled if {@link Sampler#never()} is specified. * See {@link Sampler#of(String)} for the specification string format.

*/ + @UnstableApi public static Sampler requestContextLeakDetectionSampler() { return REQUEST_CONTEXT_LEAK_DETECTION_SAMPLER; } diff --git a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java index 8c001845952..eb81e0704d2 100644 --- a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java @@ -968,9 +968,10 @@ default Path defaultMultipartUploadsLocation() { * *

The default value of this flag is {@link Sampler#never()}. * Specify the {@code -Dcom.linecorp.armeria.requestContextLeakDetectionSampler=} JVM option - * to override the default. This feature is disabled if users provide the {@link Sampler#never()}. + * to override the default. This feature is disabled if {@link Sampler#never()} is specified. * See {@link Sampler#of(String)} for the specification string format.

*/ + @UnstableApi @Nullable default Sampler requestContextLeakDetectionSampler() { return null; From f124fc8c013a5835ed699ce1834ce674efd2de01 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Fri, 3 Jun 2022 08:04:28 +0700 Subject: [PATCH 21/44] Update generic of Sampler --- .../common/LeakTracingRequestContextStorage.java | 10 +++++----- .../armeria/internal/common/RequestContextUtil.java | 5 ++--- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java b/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java index ef111b4d097..662bc683c49 100644 --- a/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java +++ b/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java @@ -37,14 +37,14 @@ public final class LeakTracingRequestContextStorage implements RequestContextSto private final RequestContextStorage delegate; private final FastThreadLocal pendingRequestCtx; - private final Sampler sampler; + private final Sampler sampler; /** * Creates a new instance. * @param delegate the underlying {@link RequestContextStorage} that stores {@link RequestContext} */ public LeakTracingRequestContextStorage(RequestContextStorage delegate) { - this(delegate, Flags.verboseExceptionSampler()); + this(delegate, Flags.requestContextLeakDetectionSampler()); } /** @@ -53,10 +53,10 @@ public LeakTracingRequestContextStorage(RequestContextStorage delegate) { * @param sampler the {@link Sampler} that determines whether to retain the stacktrace of the context leaks */ public LeakTracingRequestContextStorage(RequestContextStorage delegate, - Sampler sampler) { + Sampler sampler) { this.delegate = requireNonNull(delegate, "delegate"); pendingRequestCtx = new FastThreadLocal<>(); - this.sampler = (Sampler) requireNonNull(sampler, "sampler"); + this.sampler = requireNonNull(sampler, "sampler"); } @Nullable @@ -79,7 +79,7 @@ public T push(RequestContext toPush) { } } - if (sampler.isSampled(PendingRequestContextStackTrace.class)) { + if (sampler.isSampled(toPush)) { pendingRequestCtx.set(new PendingRequestContextStackTrace(toPush, true)); } else { pendingRequestCtx.set(new PendingRequestContextStackTrace(toPush, false)); diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java index 9e74be03886..79ce8a8e819 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java @@ -67,11 +67,10 @@ public final class RequestContextUtil { final RequestContextStorageProvider provider = Flags.requestContextStorageProvider(); final Sampler sampler = Flags.requestContextLeakDetectionSampler(); try { - if (sampler.equals(Sampler. never())) { + if (sampler == Sampler.never()) { requestContextStorage = provider.newStorage(); } else { - requestContextStorage = - new LeakTracingRequestContextStorage(provider.newStorage(), sampler); + requestContextStorage = new LeakTracingRequestContextStorage(provider.newStorage(), sampler); } } catch (Throwable t) { throw new IllegalStateException("Failed to create context storage. provider: " + provider, t); From a71a725b7573cc8190945619ab1d774e5c2e06a9 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Fri, 3 Jun 2022 23:20:10 +0700 Subject: [PATCH 22/44] Move LeakTracingRequestContextStorage to internal.common --- ...eakTracingRequestContextStorageBenchmark.java | 6 +++++- .../common/LeakTracingRequestContextStorage.java | 16 +++++----------- .../internal/common/RequestContextUtil.java | 1 - 3 files changed, 10 insertions(+), 13 deletions(-) rename benchmarks/jmh/src/jmh/java/com/linecorp/armeria/{ => internal}/common/LeakTracingRequestContextStorageBenchmark.java (93%) rename core/src/main/java/com/linecorp/armeria/{ => internal}/common/LeakTracingRequestContextStorage.java (89%) diff --git a/benchmarks/jmh/src/jmh/java/com/linecorp/armeria/common/LeakTracingRequestContextStorageBenchmark.java b/benchmarks/jmh/src/jmh/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorageBenchmark.java similarity index 93% rename from benchmarks/jmh/src/jmh/java/com/linecorp/armeria/common/LeakTracingRequestContextStorageBenchmark.java rename to benchmarks/jmh/src/jmh/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorageBenchmark.java index 4920dd991fc..511aea3d336 100644 --- a/benchmarks/jmh/src/jmh/java/com/linecorp/armeria/common/LeakTracingRequestContextStorageBenchmark.java +++ b/benchmarks/jmh/src/jmh/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorageBenchmark.java @@ -14,10 +14,14 @@ * under the License. */ -package com.linecorp.armeria.common; +package com.linecorp.armeria.internal.common; import org.openjdk.jmh.annotations.Benchmark; +import com.linecorp.armeria.common.HttpMethod; +import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.RequestContext; +import com.linecorp.armeria.common.RequestContextStorage; import com.linecorp.armeria.common.util.Sampler; import com.linecorp.armeria.server.ServiceRequestContext; diff --git a/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java b/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java similarity index 89% rename from core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java rename to core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java index 662bc683c49..c229797f4a0 100644 --- a/core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java @@ -14,13 +14,15 @@ * under the License. */ -package com.linecorp.armeria.common; +package com.linecorp.armeria.internal.common; import static com.linecorp.armeria.internal.common.RequestContextUtil.newIllegalContextPushingException; import static java.lang.Thread.currentThread; import static java.util.Objects.requireNonNull; import com.linecorp.armeria.client.ClientRequestContext; +import com.linecorp.armeria.common.RequestContext; +import com.linecorp.armeria.common.RequestContextStorage; import com.linecorp.armeria.common.annotation.Nullable; import com.linecorp.armeria.common.annotation.UnstableApi; import com.linecorp.armeria.common.util.Sampler; @@ -33,26 +35,18 @@ * information if a {@link RequestContext} is leaked. */ @UnstableApi -public final class LeakTracingRequestContextStorage implements RequestContextStorage { +final class LeakTracingRequestContextStorage implements RequestContextStorage { private final RequestContextStorage delegate; private final FastThreadLocal pendingRequestCtx; private final Sampler sampler; - /** - * Creates a new instance. - * @param delegate the underlying {@link RequestContextStorage} that stores {@link RequestContext} - */ - public LeakTracingRequestContextStorage(RequestContextStorage delegate) { - this(delegate, Flags.requestContextLeakDetectionSampler()); - } - /** * Creates a new instance. * @param delegate the underlying {@link RequestContextStorage} that stores {@link RequestContext} * @param sampler the {@link Sampler} that determines whether to retain the stacktrace of the context leaks */ - public LeakTracingRequestContextStorage(RequestContextStorage delegate, + LeakTracingRequestContextStorage(RequestContextStorage delegate, Sampler sampler) { this.delegate = requireNonNull(delegate, "delegate"); pendingRequestCtx = new FastThreadLocal<>(); diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java index 79ce8a8e819..a237ce95036 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java @@ -33,7 +33,6 @@ import com.linecorp.armeria.client.DefaultClientRequestContext; import com.linecorp.armeria.common.Flags; import com.linecorp.armeria.common.HttpRequest; -import com.linecorp.armeria.common.LeakTracingRequestContextStorage; import com.linecorp.armeria.common.RequestContext; import com.linecorp.armeria.common.RequestContextStorage; import com.linecorp.armeria.common.RequestContextStorageProvider; From c4a534fdedeebd442479fab9581a557dff2e6afa Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Fri, 3 Jun 2022 23:21:36 +0700 Subject: [PATCH 23/44] Correct spelling --- .../linecorp/armeria/common/TraceRequestContextLeakTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java index 1fdf6ad9b1d..f9951dde09d 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java @@ -79,7 +79,7 @@ void singleThreadContextNotLeak() throws InterruptedException { @Test @SuppressWarnings("MustBeClosedChecker") - void singleTreadContextLeak() throws InterruptedException { + void singleThreadContextLeak() throws InterruptedException { final AtomicBoolean isThrown = new AtomicBoolean(); final AtomicReference exception = new AtomicReference<>(); From 3b00c2a1917fd558e60b9ee018b9cb15830a030b Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Sun, 12 Jun 2022 21:16:43 +0700 Subject: [PATCH 24/44] Modifying Samplers.of(String) to accept true and false --- .../java/com/linecorp/armeria/common/Flags.java | 4 ---- .../common/SystemPropertyFlagsProvider.java | 14 -------------- .../com/linecorp/armeria/common/util/Samplers.java | 2 ++ .../com/linecorp/armeria/common/FlagsTest.java | 2 +- .../linecorp/armeria/common/util/SamplerTest.java | 4 ++++ 5 files changed, 7 insertions(+), 19 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/Flags.java b/core/src/main/java/com/linecorp/armeria/common/Flags.java index 9a995d02188..fa8120a46b8 100644 --- a/core/src/main/java/com/linecorp/armeria/common/Flags.java +++ b/core/src/main/java/com/linecorp/armeria/common/Flags.java @@ -113,10 +113,6 @@ public final class Flags { static { final String strSpec = getNormalized("verboseExceptions", DefaultFlagsProvider.VERBOSE_EXCEPTION_SAMPLER_SPEC, val -> { - if ("true".equals(val) || "false".equals(val)) { - return true; - } - try { Sampler.of(val); return true; diff --git a/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java index f6fc20907bf..a05f58d757e 100644 --- a/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java @@ -71,20 +71,12 @@ public Sampler> verboseExceptionSampler() { if (spec == null) { return null; } - if ("true".equals(spec) || "always".equals(spec)) { - return Sampler.always(); - } - if ("false".equals(spec) || "never".equals(spec)) { - return Sampler.never(); - } - try { Sampler.of(spec); } catch (Exception e) { // Invalid sampler specification throw new IllegalArgumentException("invalid sampler spec: " + spec, e); } - return new ExceptionSampler(spec); } @@ -444,12 +436,6 @@ public Sampler requestContextLeakDetectionSampler() { if (spec == null) { return null; } - if ("true".equals(spec) || "always".equals(spec)) { - return Sampler.always(); - } - if ("false".equals(spec) || "never".equals(spec)) { - return Sampler.never(); - } try { return Sampler.of(spec); } catch (Exception e) { diff --git a/core/src/main/java/com/linecorp/armeria/common/util/Samplers.java b/core/src/main/java/com/linecorp/armeria/common/util/Samplers.java index 00436d55f49..1d8463d5426 100644 --- a/core/src/main/java/com/linecorp/armeria/common/util/Samplers.java +++ b/core/src/main/java/com/linecorp/armeria/common/util/Samplers.java @@ -70,8 +70,10 @@ static Sampler of(String specification) { requireNonNull(specification, "specification"); switch (specification.trim()) { case "always": + case "true": return Sampler.always(); case "never": + case "false": return Sampler.never(); } diff --git a/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java b/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java index 9aa212d3e52..2719d3a0c45 100644 --- a/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java +++ b/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java @@ -210,7 +210,7 @@ void systemPropertyVerboseExceptionSampler() throws Throwable { final Method method = flags.getDeclaredMethod("verboseExceptionSampler"); assertThat(method.invoke(flags)) .usingRecursiveComparison() - .isEqualTo(Sampler.always()); + .isEqualTo(new ExceptionSampler("true")); } @Test diff --git a/core/src/test/java/com/linecorp/armeria/common/util/SamplerTest.java b/core/src/test/java/com/linecorp/armeria/common/util/SamplerTest.java index 92183c934ed..3e3c5302508 100644 --- a/core/src/test/java/com/linecorp/armeria/common/util/SamplerTest.java +++ b/core/src/test/java/com/linecorp/armeria/common/util/SamplerTest.java @@ -31,10 +31,14 @@ void goodOf() { // 'always' assertThat(Sampler.of("always")).isSameAs(Sampler.always()); assertThat(Sampler.of(" always ")).isSameAs(Sampler.always()); + assertThat(Sampler.of("true")).isSameAs(Sampler.always()); + assertThat(Sampler.of(" true ")).isSameAs(Sampler.always()); // 'never' assertThat(Sampler.of("never")).isSameAs(Sampler.never()); assertThat(Sampler.of(" never ")).isSameAs(Sampler.never()); + assertThat(Sampler.of("false")).isSameAs(Sampler.never()); + assertThat(Sampler.of(" false ")).isSameAs(Sampler.never()); // 'random=' assertThat(Sampler.of("random=0")).isSameAs(Sampler.never()); From af31a6ef583cbdf356ce8e836597419a44f02f77 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Sat, 18 Jun 2022 18:41:19 +0700 Subject: [PATCH 25/44] Add illegallyPushServiceRequestContext test --- .../armeria/common/TraceRequestContextLeakTest.java | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java index f9951dde09d..b7dcd9a7779 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java @@ -17,6 +17,7 @@ package com.linecorp.armeria.common; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.awaitility.Awaitility.await; import java.util.concurrent.ConcurrentHashMap; @@ -187,6 +188,16 @@ void multiThreadContextLeak() throws InterruptedException { } } + @Test + void illegallyPushServiceRequestContext() { + final ServiceRequestContext sctx1 = newCtx("/1"); + final ServiceRequestContext sctx2 = newCtx("/2"); + try (SafeCloseable ignored = sctx1.push()) { + assertThatThrownBy(sctx2::push).isInstanceOf(IllegalStateException.class) + .getRootCause().hasMessageContaining("RequestContext didn't popped"); + } + } + private static ServiceRequestContext newCtx(String path) { return ServiceRequestContext.builder(HttpRequest.of(HttpMethod.GET, path)) .build(); From ab4de8489a3e4c3d93d76e60db7faaa6f45e8293 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Sat, 2 Jul 2022 00:10:32 +0700 Subject: [PATCH 26/44] Refactor LeakTracingRequestContextStorage --- .../armeria/client/ClientRequestContext.java | 10 +- .../armeria/common/RequestContextWrapper.java | 10 + .../ThreadLocalRequestContextStorage.java | 2 +- .../LeakTracingRequestContextStorage.java | 94 +++--- .../internal/common/RequestContextUtil.java | 18 +- .../armeria/server/ServiceRequestContext.java | 10 +- .../common/ClientRequestContextTest.java | 278 ++++++++++++++++++ .../EnableLeakDetectionFlagsProvider.java | 4 +- .../common/ServiceRequestContextTest.java | 48 +-- .../common/TraceRequestContextLeakTest.java | 97 +++++- .../com.linecorp.armeria.common.FlagsProvider | 2 +- 11 files changed, 477 insertions(+), 96 deletions(-) create mode 100644 it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ClientRequestContextTest.java rename it/trace-context-leak/src/test/java/com/linecorp/armeria/{ => internal}/common/EnableLeakDetectionFlagsProvider.java (86%) rename it/trace-context-leak/src/test/java/com/linecorp/armeria/{ => internal}/common/ServiceRequestContextTest.java (88%) rename it/trace-context-leak/src/test/java/com/linecorp/armeria/{ => internal}/common/TraceRequestContextLeakTest.java (65%) diff --git a/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java b/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java index 791d743ce9d..2185f373991 100644 --- a/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java +++ b/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java @@ -214,15 +214,15 @@ static ClientRequestContextBuilder builder(RpcRequest request, URI uri) { @MustBeClosed default SafeCloseable push() { final RequestContext oldCtx = RequestContextUtil.getAndSet(this); - if (oldCtx == this) { - // Reentrance - return noopSafeCloseable(); - } - if (oldCtx == null) { return RequestContextUtil.invokeHookAndPop(this, null); } + if (oldCtx.equals(this)) { + // Reentrance + return noopSafeCloseable(); + } + final ServiceRequestContext root = root(); if (oldCtx.root() == root) { return RequestContextUtil.invokeHookAndPop(this, oldCtx); diff --git a/core/src/main/java/com/linecorp/armeria/common/RequestContextWrapper.java b/core/src/main/java/com/linecorp/armeria/common/RequestContextWrapper.java index 1655bf8b219..02fef371f46 100644 --- a/core/src/main/java/com/linecorp/armeria/common/RequestContextWrapper.java +++ b/core/src/main/java/com/linecorp/armeria/common/RequestContextWrapper.java @@ -218,4 +218,14 @@ public ByteBufAllocator alloc() { public String toString() { return delegate().toString(); } + + @Override + public boolean equals(Object obj) { + return delegate().equals(obj); + } + + @Override + public int hashCode() { + return delegate().hashCode(); + } } diff --git a/core/src/main/java/com/linecorp/armeria/common/ThreadLocalRequestContextStorage.java b/core/src/main/java/com/linecorp/armeria/common/ThreadLocalRequestContextStorage.java index 7fe5e0a59aa..3a8239892d1 100644 --- a/core/src/main/java/com/linecorp/armeria/common/ThreadLocalRequestContextStorage.java +++ b/core/src/main/java/com/linecorp/armeria/common/ThreadLocalRequestContextStorage.java @@ -46,7 +46,7 @@ public void pop(RequestContext current, @Nullable RequestContext toRestore) { requireNonNull(current, "current"); final InternalThreadLocalMap map = InternalThreadLocalMap.get(); final RequestContext contextInThreadLocal = context.get(map); - if (current != contextInThreadLocal) { + if (!contextInThreadLocal.equals(current)) { throw newIllegalContextPoppingException(current, contextInThreadLocal); } context.set(map, toRestore); diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java b/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java index c229797f4a0..9da172c5601 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java @@ -16,19 +16,22 @@ package com.linecorp.armeria.internal.common; -import static com.linecorp.armeria.internal.common.RequestContextUtil.newIllegalContextPushingException; import static java.lang.Thread.currentThread; import static java.util.Objects.requireNonNull; +import java.io.PrintWriter; +import java.io.StringWriter; + import com.linecorp.armeria.client.ClientRequestContext; +import com.linecorp.armeria.client.ClientRequestContextWrapper; import com.linecorp.armeria.common.RequestContext; import com.linecorp.armeria.common.RequestContextStorage; +import com.linecorp.armeria.common.RequestContextWrapper; import com.linecorp.armeria.common.annotation.Nullable; import com.linecorp.armeria.common.annotation.UnstableApi; import com.linecorp.armeria.common.util.Sampler; import com.linecorp.armeria.server.ServiceRequestContext; - -import io.netty.util.concurrent.FastThreadLocal; +import com.linecorp.armeria.server.ServiceRequestContextWrapper; /** * A {@link RequestContextStorage} which keeps track of {@link RequestContext}s, reporting pushed thread @@ -38,7 +41,6 @@ final class LeakTracingRequestContextStorage implements RequestContextStorage { private final RequestContextStorage delegate; - private final FastThreadLocal pendingRequestCtx; private final Sampler sampler; /** @@ -49,7 +51,6 @@ final class LeakTracingRequestContextStorage implements RequestContextStorage { LeakTracingRequestContextStorage(RequestContextStorage delegate, Sampler sampler) { this.delegate = requireNonNull(delegate, "delegate"); - pendingRequestCtx = new FastThreadLocal<>(); this.sampler = requireNonNull(sampler, "sampler"); } @@ -57,38 +58,16 @@ final class LeakTracingRequestContextStorage implements RequestContextStorage { @Override public T push(RequestContext toPush) { requireNonNull(toPush, "toPush"); - - final RequestContext prevContext = delegate.currentOrNull(); - if (prevContext != null) { - if (prevContext == toPush) { - // Re-entrance - } else if (toPush instanceof ServiceRequestContext && - prevContext.root() == toPush) { - // The delegate has the ServiceRequestContext whose root() is toPush - } else if (toPush instanceof ClientRequestContext && - prevContext.root() == toPush.root()) { - // The delegate has the ClientRequestContext whose root() is the same as toPush.root() - } else { - throw newIllegalContextPushingException(prevContext, toPush, pendingRequestCtx.get()); - } - } - if (sampler.isSampled(toPush)) { - pendingRequestCtx.set(new PendingRequestContextStackTrace(toPush, true)); - } else { - pendingRequestCtx.set(new PendingRequestContextStackTrace(toPush, false)); + return delegate.push(warpRequestContext(toPush)); } - return delegate.push(toPush); } @Override public void pop(RequestContext current, @Nullable RequestContext toRestore) { - try { - delegate.pop(current, toRestore); - } finally { - pendingRequestCtx.remove(); - } + requireNonNull(current, "current"); + delegate.pop(current, toRestore); } @Nullable @@ -97,14 +76,59 @@ public T currentOrNull() { return delegate.currentOrNull(); } - static class PendingRequestContextStackTrace extends RuntimeException { + private static RequestContextWrapper warpRequestContext(RequestContext ctx) { + return ctx instanceof ClientRequestContext ? + new TraceableClientRequestContext((ClientRequestContext) ctx) + : new TraceableServiceRequestContext((ServiceRequestContext) ctx); + } + + private static final class TraceableClientRequestContext extends ClientRequestContextWrapper { + + private final PendingRequestContextStackTrace stackTrace; + + private TraceableClientRequestContext(ClientRequestContext delegate) { + super(delegate); + stackTrace = new PendingRequestContextStackTrace(); + } + + @Override + public String toString() { + final StringWriter sw = new StringWriter(); + stackTrace.printStackTrace(new PrintWriter(sw)); + return new StringBuilder().append(getClass().getSimpleName()) + .append(super.toString()) + .append(System.lineSeparator()) + .append(sw).toString(); + } + } + + private static final class TraceableServiceRequestContext extends ServiceRequestContextWrapper { + + private final PendingRequestContextStackTrace stackTrace; + + private TraceableServiceRequestContext(ServiceRequestContext delegate) { + super(delegate); + stackTrace = new PendingRequestContextStackTrace(); + } + + @Override + public String toString() { + final StringWriter sw = new StringWriter(); + stackTrace.printStackTrace(new PrintWriter(sw)); + return new StringBuilder().append(getClass().getSimpleName()) + .append(super.toString()) + .append(System.lineSeparator()) + .append(sw).toString(); + } + } + + private static final class PendingRequestContextStackTrace extends RuntimeException { private static final long serialVersionUID = -689451606253441556L; - PendingRequestContextStackTrace(RequestContext context, boolean isSample) { - super("At thread [" + currentThread().getName() + "], previous RequestContext didn't popped : " + - context + (isSample ? ", It is pushed at the following stacktrace" : ""), null, - true, isSample); + private PendingRequestContextStackTrace() { + super("At thread [" + currentThread().getName() + "] previous RequestContext is pushed at " + + "the following stacktrace"); } } } diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java index a237ce95036..13b07af3171 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java @@ -95,22 +95,12 @@ public static SafeCloseable noopSafeCloseable() { */ public static IllegalStateException newIllegalContextPushingException( RequestContext newCtx, RequestContext oldCtx) { - return newIllegalContextPushingException(newCtx, oldCtx, null); - } - - /** - * Returns an {@link IllegalStateException} which is raised when pushing a context from - * the unexpected thread or forgetting to close the previous context. - */ - public static IllegalStateException newIllegalContextPushingException( - RequestContext newCtx, RequestContext oldCtx, @Nullable Throwable cause) { requireNonNull(newCtx, "newCtx"); requireNonNull(oldCtx, "oldCtx"); - final String message = "Trying to call object wrapped with context " + newCtx + ", but context is " + - "currently set to " + oldCtx + ". This means the callback was called from " + - "unexpected thread or forgetting to close previous context."; - final IllegalStateException ex = cause == null ? new IllegalStateException(message) - : new IllegalStateException(message, cause); + final IllegalStateException ex = new IllegalStateException( + "Trying to call object wrapped with context " + newCtx + ", but context is currently " + + "set to " + oldCtx + ". This means the callback was called from " + + "unexpected thread or forgetting to close previous context."); if (REPORTED_THREADS.add(Thread.currentThread())) { logger.warn("An error occurred while pushing a context", ex); } diff --git a/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java b/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java index fad5e162b5f..4d729b8dc16 100644 --- a/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java +++ b/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java @@ -218,15 +218,15 @@ default ServiceRequestContext root() { @MustBeClosed default SafeCloseable push() { final RequestContext oldCtx = RequestContextUtil.getAndSet(this); - if (oldCtx == this) { - // Reentrance - return noopSafeCloseable(); - } - if (oldCtx == null) { return RequestContextUtil.invokeHookAndPop(this, null); } + if (oldCtx.equals(this)) { + // Reentrance + return noopSafeCloseable(); + } + if (oldCtx.root() == this) { return RequestContextUtil.invokeHookAndPop(this, oldCtx); } diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ClientRequestContextTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ClientRequestContextTest.java new file mode 100644 index 00000000000..887e65e631b --- /dev/null +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ClientRequestContextTest.java @@ -0,0 +1,278 @@ +/* + * Copyright 2019 LINE Corporation + * + * LINE Corporation licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ +package com.linecorp.armeria.internal.common; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.util.function.Function; + +import org.junit.jupiter.api.Test; + +import com.linecorp.armeria.client.ClientRequestContext; +import com.linecorp.armeria.common.HttpMethod; +import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.RequestContext; +import com.linecorp.armeria.common.annotation.Nullable; +import com.linecorp.armeria.common.util.SafeCloseable; +import com.linecorp.armeria.server.ServiceRequestContext; + +import io.netty.util.AttributeKey; + +/** + * This is copy from ClientRequestContextTest on core module to test behaviour consistency. + */ +class ClientRequestContextTest { + + @Test + void current() { + assertThatThrownBy(ClientRequestContext::current).isInstanceOf(IllegalStateException.class) + .hasMessageContaining("unavailable"); + + final ClientRequestContext ctx = clientRequestContext(); + assertThat(ctx.id()).isNotNull(); + try (SafeCloseable unused = ctx.push()) { + assertThat(ClientRequestContext.current()).isEqualTo(ctx); + } + assertCurrentCtx(null); + + try (SafeCloseable unused = serviceRequestContext().push()) { + assertThatThrownBy(ClientRequestContext::current) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("not a client-side context"); + } + } + + @Test + void currentOrNull() { + assertThat(ClientRequestContext.currentOrNull()).isNull(); + + final ClientRequestContext ctx = clientRequestContext(); + try (SafeCloseable unused = ctx.push()) { + assertThat(ClientRequestContext.currentOrNull()).isEqualTo(ctx); + } + assertCurrentCtx(null); + + try (SafeCloseable unused = serviceRequestContext().push()) { + assertThat(ClientRequestContext.currentOrNull()).isNull(); + } + } + + @Test + void mapCurrent() { + assertThat(ClientRequestContext.mapCurrent(ctx -> "foo", () -> "bar")).isEqualTo("bar"); + assertThat(ClientRequestContext.mapCurrent(Function.identity(), null)).isNull(); + + final ClientRequestContext ctx = clientRequestContext(); + try (SafeCloseable unused = ctx.push()) { + assertThat(ClientRequestContext.mapCurrent(c -> "foo", () -> "bar")).isEqualTo("foo"); + assertThat(ClientRequestContext.mapCurrent(Function.identity(), null)).isEqualTo(ctx); + } + assertCurrentCtx(null); + + try (SafeCloseable unused = serviceRequestContext().push()) { + assertThatThrownBy(() -> ClientRequestContext.mapCurrent(c -> "foo", () -> "bar")) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("not a client-side context"); + } + } + + @Test + void pushReentrance() { + final ClientRequestContext ctx = clientRequestContext(); + try (SafeCloseable ignored = ctx.push()) { + assertCurrentCtx(ctx); + try (SafeCloseable ignored2 = ctx.push()) { + assertCurrentCtx(ctx); + } + assertCurrentCtx(ctx); + } + assertCurrentCtx(null); + } + + @Test + void pushWithOldServiceCtx() { + final ServiceRequestContext sctx = serviceRequestContext(); + try (SafeCloseable ignored = sctx.push()) { + assertCurrentCtx(sctx); + // The root of ClientRequestContext is sctx. + final ClientRequestContext cctx = clientRequestContext(); + try (SafeCloseable ignored1 = cctx.push()) { + assertCurrentCtx(cctx); + try (SafeCloseable ignored2 = sctx.push()) { + assertCurrentCtx(sctx); + } + assertCurrentCtx(cctx); + } + assertCurrentCtx(sctx); + } + assertCurrentCtx(null); + } + + @Test + void pushWithOldServiceCtx_exceptionWhenServiceCtxIsDifferFromRoot() { + final ServiceRequestContext sctx1 = serviceRequestContext(); + final ClientRequestContext ctx; + try (SafeCloseable ignored = sctx1.push()) { + ctx = clientRequestContext(); + } + final ServiceRequestContext sctx2 = serviceRequestContext(); + try (SafeCloseable ignored = sctx2.push()) { + assertThatThrownBy(ctx::push).isInstanceOf(IllegalStateException.class); + } + } + + @Test + void pushWithOldClientCtxWhoseRootIsSameServiceCtx_ctx2IsCreatedSameLayer() { + final ServiceRequestContext sctx = serviceRequestContext(); + try (SafeCloseable ignored = sctx.push()) { + assertCurrentCtx(sctx); + final ClientRequestContext cctx1 = clientRequestContext(); + final ClientRequestContext cctx2 = clientRequestContext(); + assertThat(cctx1.root()).isEqualTo(cctx2.root()); + + try (SafeCloseable ignored1 = cctx1.push()) { + assertCurrentCtx(cctx1); + try (SafeCloseable ignored2 = cctx2.push()) { + assertCurrentCtx(cctx2); + } + assertCurrentCtx(cctx1); + } + assertCurrentCtx(sctx); + } + assertCurrentCtx(null); + } + + @Test + void pushWithOldClientCtxWhoseRootIsSameServiceCtx__ctx2IsCreatedUnderCtx1() { + final ServiceRequestContext sctx = serviceRequestContext(); + try (SafeCloseable ignored = sctx.push()) { + assertCurrentCtx(sctx); + final ClientRequestContext cctx1 = clientRequestContext(); + try (SafeCloseable ignored1 = cctx1.push()) { + assertCurrentCtx(cctx1); + final ClientRequestContext cctx2 = clientRequestContext(); + assertThat(cctx1.root()).isEqualTo(cctx2.root()); + + try (SafeCloseable ignored2 = cctx2.push()) { + assertCurrentCtx(cctx2); + } + assertCurrentCtx(cctx1); + } + assertCurrentCtx(sctx); + } + assertCurrentCtx(null); + } + + @Test + void pushWithOldClientCtxWhoseRootIsSameServiceCtx_derivedCtx() { + final ServiceRequestContext sctx = serviceRequestContext(); + try (SafeCloseable ignored = sctx.push()) { + assertCurrentCtx(sctx); + final ClientRequestContext cctx1 = clientRequestContext(); + final ClientRequestContext derived = cctx1.newDerivedContext(cctx1.id(), cctx1.request(), + cctx1.rpcRequest(), cctx1.endpoint()); + try (SafeCloseable ignored1 = derived.push()) { + assertCurrentCtx(derived); + final ClientRequestContext cctx2 = clientRequestContext(); + assertThat(derived.root()).isEqualTo(cctx2.root()); + + try (SafeCloseable ignored2 = cctx2.push()) { + assertCurrentCtx(cctx2); + } + assertCurrentCtx(derived); + } + assertCurrentCtx(sctx); + } + assertCurrentCtx(null); + } + + @Test + void pushWithOldClientCtxWhoseRootIsDifferent() { + final ServiceRequestContext sctx1 = serviceRequestContext(); + final ClientRequestContext cctx1; + try (SafeCloseable ignored = sctx1.push()) { + cctx1 = clientRequestContext(); + } + final ServiceRequestContext sctx2 = serviceRequestContext(); + final ClientRequestContext cctx2; + try (SafeCloseable ignored = sctx2.push()) { + cctx2 = clientRequestContext(); + } + try (SafeCloseable ignored = cctx1.push()) { + assertThatThrownBy(cctx2::push).isInstanceOf(IllegalStateException.class); + } + } + + @Test + void pushWithOldClientCtxWhoseRootIsNull() { + final ClientRequestContext cctx1 = clientRequestContext(); + try (SafeCloseable ignored1 = cctx1.push()) { + assertCurrentCtx(cctx1); + final ClientRequestContext cctx2 = clientRequestContext(); + assertThat(cctx1.root()).isNull(); + assertThat(cctx2.root()).isNull(); + try (SafeCloseable ignored2 = cctx2.push()) { + assertCurrentCtx(cctx2); + } + assertCurrentCtx(cctx1); + } + assertCurrentCtx(null); + } + + @Test + void hasAttr() { + final AttributeKey key = AttributeKey.valueOf(ClientRequestContextTest.class, "KEY"); + final ServiceRequestContext sctx = serviceRequestContext(); + try (SafeCloseable ignored = sctx.push()) { + final ClientRequestContext cctx = clientRequestContext(); + assertThat(sctx.hasAttr(key)).isFalse(); + assertThat(cctx.hasAttr(key)).isFalse(); + + sctx.setAttr(key, "foo"); + assertThat(sctx.hasAttr(key)).isTrue(); + assertThat(cctx.hasAttr(key)).isTrue(); + } + } + + @Test + void hasOwnAttr() { + final AttributeKey key = AttributeKey.valueOf(ClientRequestContextTest.class, "KEY"); + final ServiceRequestContext sctx = serviceRequestContext(); + try (SafeCloseable ignored = sctx.push()) { + final ClientRequestContext cctx = clientRequestContext(); + assertThat(sctx.hasOwnAttr(key)).isFalse(); + assertThat(cctx.hasOwnAttr(key)).isFalse(); + + sctx.setAttr(key, "foo"); + assertThat(sctx.hasOwnAttr(key)).isTrue(); + assertThat(cctx.hasOwnAttr(key)).isFalse(); + } + } + + private static void assertCurrentCtx(@Nullable RequestContext ctx) { + final RequestContext current = RequestContext.currentOrNull(); + assertThat(current).isEqualTo(ctx); + } + + private static ServiceRequestContext serviceRequestContext() { + return ServiceRequestContext.of(HttpRequest.of(HttpMethod.GET, "/")); + } + + private static ClientRequestContext clientRequestContext() { + return ClientRequestContext.of(HttpRequest.of(HttpMethod.GET, "/")); + } +} diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/EnableLeakDetectionFlagsProvider.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/EnableLeakDetectionFlagsProvider.java similarity index 86% rename from it/trace-context-leak/src/test/java/com/linecorp/armeria/common/EnableLeakDetectionFlagsProvider.java rename to it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/EnableLeakDetectionFlagsProvider.java index 52171ae954c..720e46ffb20 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/EnableLeakDetectionFlagsProvider.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/EnableLeakDetectionFlagsProvider.java @@ -14,8 +14,10 @@ * under the License. */ -package com.linecorp.armeria.common; +package com.linecorp.armeria.internal.common; +import com.linecorp.armeria.common.FlagsProvider; +import com.linecorp.armeria.common.RequestContext; import com.linecorp.armeria.common.util.Sampler; public final class EnableLeakDetectionFlagsProvider implements FlagsProvider { diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/ServiceRequestContextTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ServiceRequestContextTest.java similarity index 88% rename from it/trace-context-leak/src/test/java/com/linecorp/armeria/common/ServiceRequestContextTest.java rename to it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ServiceRequestContextTest.java index 760ed5bdc70..3b23e4f3637 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/ServiceRequestContextTest.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ServiceRequestContextTest.java @@ -14,7 +14,7 @@ * under the License. */ -package com.linecorp.armeria.common; +package com.linecorp.armeria.internal.common; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -26,6 +26,10 @@ import com.google.common.collect.ImmutableList; import com.linecorp.armeria.client.ClientRequestContext; +import com.linecorp.armeria.common.HttpMethod; +import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.QueryParams; +import com.linecorp.armeria.common.RequestContext; import com.linecorp.armeria.common.annotation.Nullable; import com.linecorp.armeria.common.util.SafeCloseable; import com.linecorp.armeria.server.ServiceRequestContext; @@ -40,14 +44,14 @@ void current() { assertThatThrownBy(ServiceRequestContext::current).isInstanceOf(IllegalStateException.class) .hasMessageContaining("unavailable"); - final ServiceRequestContext sctx = serviceRequestContext(); + final ServiceRequestContext sctx = serviceRequestContext("/1"); try (SafeCloseable unused = sctx.push()) { assertThat(ServiceRequestContext.current()).isSameAs(sctx); - final ClientRequestContext cctx = clientRequestContext(); + final ClientRequestContext cctx = clientRequestContext("/2"); try (SafeCloseable unused1 = cctx.push()) { - assertThat(ServiceRequestContext.current()).isSameAs(sctx); - assertThat(ClientRequestContext.current()).isSameAs(cctx); - assertThat((ClientRequestContext) RequestContext.current()).isSameAs(cctx); + assertThat(ServiceRequestContext.current()).isEqualTo(sctx); + assertThat(ClientRequestContext.current()).isEqualTo(cctx); + assertThat((ClientRequestContext) RequestContext.current()).isEqualTo(cctx); } assertCurrentCtx(sctx); } @@ -69,9 +73,9 @@ void currentOrNull() { assertThat(ServiceRequestContext.currentOrNull()).isSameAs(sctx); final ClientRequestContext cctx = clientRequestContext(); try (SafeCloseable unused1 = cctx.push()) { - assertThat(ServiceRequestContext.currentOrNull()).isSameAs(sctx); - assertThat(ClientRequestContext.current()).isSameAs(cctx); - assertThat((ClientRequestContext) RequestContext.current()).isSameAs(cctx); + assertThat(ServiceRequestContext.currentOrNull()).isEqualTo(sctx); + assertThat(ClientRequestContext.current()).isEqualTo(cctx); + assertThat((ClientRequestContext) RequestContext.current()).isEqualTo(cctx); } assertCurrentCtx(sctx); } @@ -88,23 +92,23 @@ void mapCurrent() { .isEqualTo("defaultValue"); assertThat(ServiceRequestContext.mapCurrent(Function.identity(), null)).isNull(); - final ServiceRequestContext sctx = serviceRequestContext(); + final ServiceRequestContext sctx = serviceRequestContext("/1"); try (SafeCloseable unused = sctx.push()) { assertThat(ServiceRequestContext.mapCurrent(c -> c == sctx ? "foo" : "bar", () -> "defaultValue")) .isEqualTo("foo"); assertThat(ServiceRequestContext.mapCurrent(Function.identity(), null)).isSameAs(sctx); - final ClientRequestContext cctx = clientRequestContext(); + final ClientRequestContext cctx = clientRequestContext("/2"); try (SafeCloseable unused1 = cctx.push()) { assertThat(ServiceRequestContext.mapCurrent(c -> c == sctx ? "foo" : "bar", () -> "defaultValue")) .isEqualTo("foo"); - assertThat(ClientRequestContext.mapCurrent(c -> c == cctx ? "baz" : "qux", + assertThat(ClientRequestContext.mapCurrent(c -> c.equals(cctx) ? "baz" : "qux", () -> "defaultValue")) .isEqualTo("baz"); - assertThat(ServiceRequestContext.mapCurrent(Function.identity(), null)).isSameAs(sctx); - assertThat(ClientRequestContext.mapCurrent(Function.identity(), null)).isSameAs(cctx); - assertThat(RequestContext.mapCurrent(Function.identity(), null)).isSameAs(cctx); + assertThat(ServiceRequestContext.mapCurrent(Function.identity(), null)).isEqualTo(sctx); + assertThat(ClientRequestContext.mapCurrent(Function.identity(), null)).isEqualTo(cctx); + assertThat(RequestContext.mapCurrent(Function.identity(), null)).isEqualTo(cctx); } assertCurrentCtx(sctx); } @@ -197,14 +201,22 @@ void queryParams() { private static void assertCurrentCtx(@Nullable RequestContext ctx) { final RequestContext current = RequestContext.currentOrNull(); - assertThat(current).isSameAs(ctx); + assertThat(current).isEqualTo(ctx); } private static ServiceRequestContext serviceRequestContext() { - return ServiceRequestContext.of(HttpRequest.of(HttpMethod.GET, "/")); + return serviceRequestContext("/"); + } + + private static ServiceRequestContext serviceRequestContext(String path) { + return ServiceRequestContext.of(HttpRequest.of(HttpMethod.GET, path)); } private static ClientRequestContext clientRequestContext() { - return ClientRequestContext.of(HttpRequest.of(HttpMethod.GET, "/")); + return clientRequestContext("/"); + } + + private static ClientRequestContext clientRequestContext(String path) { + return ClientRequestContext.of(HttpRequest.of(HttpMethod.GET, path)); } } diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/TraceRequestContextLeakTest.java similarity index 65% rename from it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java rename to it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/TraceRequestContextLeakTest.java index b7dcd9a7779..909afe32623 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/TraceRequestContextLeakTest.java @@ -14,12 +14,14 @@ * under the License. */ -package com.linecorp.armeria.common; +package com.linecorp.armeria.internal.common; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.awaitility.Awaitility.await; +import java.util.HashSet; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; import java.util.concurrent.Executor; @@ -29,6 +31,10 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; +import com.linecorp.armeria.client.ClientRequestContext; +import com.linecorp.armeria.common.HttpMethod; +import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.RequestContext; import com.linecorp.armeria.common.util.SafeCloseable; import com.linecorp.armeria.server.ServiceRequestContext; import com.linecorp.armeria.testing.junit5.common.EventLoopExtension; @@ -66,7 +72,10 @@ void singleThreadContextNotLeak() throws InterruptedException { executor.execute(() -> { final ServiceRequestContext anotherCtx = newCtx("/2"); try (SafeCloseable ignore = anotherCtx.push()) { - //Ignore + final ClientRequestContext clientCtx = newClientCtx("/3"); + try (SafeCloseable ignore2 = clientCtx.push()) { + //Ignore + } } catch (Exception ex) { isThrown.set(true); } finally { @@ -89,7 +98,7 @@ void singleThreadContextLeak() throws InterruptedException { executor.execute(() -> { final ServiceRequestContext ctx = newCtx("/1"); - final SafeCloseable leaked = ctx.push(); + final SafeCloseable leaked = ctx.push(); // <- Leaked, should show in error. deferredClose.add(executor, leaked); }); @@ -104,7 +113,8 @@ void singleThreadContextLeak() throws InterruptedException { }); await().untilTrue(isThrown); - assertThat(exception.get()).getRootCause().hasMessageContaining("RequestContext didn't popped"); + assertThat(exception.get()) + .hasMessageContaining("singleThreadContextLeak$2(TraceRequestContextLeakTest.java:101)"); } } @@ -116,19 +126,25 @@ void multiThreadContextLeakNotInterfereOthersEventLoop() throws InterruptedExcep final EventLoopGroup executor = eventLoopGroupExtension.get(); + final Executor ex1 = executor.next(); + final Executor ex2 = executor.next(); + try (DeferredClose deferredClose = new DeferredClose()) { - executor.next().execute(() -> { + ex1.execute(() -> { final ServiceRequestContext ctx = newCtx("/1"); final SafeCloseable leaked = ctx.push(); latch.countDown(); deferredClose.add(executor, leaked); }); - executor.next().execute(() -> { + ex2.execute(() -> { //Leak happened on the first eventLoop shouldn't affect 2nd eventLoop when trying to push final ServiceRequestContext anotherCtx = newCtx("/2"); - try (SafeCloseable ignore = anotherCtx.push()) { - //Ignore + try (SafeCloseable ignore1 = anotherCtx.push()) { + final ClientRequestContext cctx = newClientCtx("/3"); + try (SafeCloseable ignore2 = cctx.push()) { + //Ignore + } } catch (Exception ex) { //Not suppose to throw exception on the second event loop isThrown.set(true); @@ -150,21 +166,21 @@ void multiThreadContextLeak() throws InterruptedException { final EventLoopGroup executor = eventLoopGroupExtension.get(); - final ServiceRequestContext ctx = newCtx("/1"); - final ServiceRequestContext anotherCtx = newCtx("/2"); + final ServiceRequestContext ctx = newCtx("/1-leak"); + final ServiceRequestContext anotherCtx = newCtx("/2-leak"); + final ServiceRequestContext anotherCtx3 = newCtx("/3-leak"); final Executor ex1 = executor.next(); final Executor ex2 = executor.next(); try (DeferredClose deferredClose = new DeferredClose()) { ex1.execute(() -> { - final SafeCloseable leaked = ctx.push(); + final SafeCloseable leaked = ctx.push(); // <- Leaked, should show in error. deferredClose.add(ex1, leaked); }); ex2.execute(() -> { try { - //Intentional leak final SafeCloseable leaked = anotherCtx.push(); deferredClose.add(ex2, leaked); } catch (Exception ex) { @@ -175,7 +191,7 @@ void multiThreadContextLeak() throws InterruptedException { assertThat(isThrown).isFalse(); ex1.execute(() -> { - try (SafeCloseable ignore = anotherCtx.push()) { + try (SafeCloseable ignore = anotherCtx3.push()) { //Ignore } catch (Exception ex) { isThrown.set(true); @@ -184,41 +200,90 @@ void multiThreadContextLeak() throws InterruptedException { }); await().untilTrue(isThrown); - assertThat(exception.get()).getRootCause().hasMessageContaining("RequestContext didn't popped"); + assertThat(exception.get()) + .hasMessageContaining("multiThreadContextLeak$6(TraceRequestContextLeakTest.java:178)"); } } @Test - void illegallyPushServiceRequestContext() { + void pushIllegalServiceRequestContext() { final ServiceRequestContext sctx1 = newCtx("/1"); final ServiceRequestContext sctx2 = newCtx("/2"); try (SafeCloseable ignored = sctx1.push()) { assertThatThrownBy(sctx2::push).isInstanceOf(IllegalStateException.class) - .getRootCause().hasMessageContaining("RequestContext didn't popped"); + .hasMessageContaining("pushed at the following stacktrace"); } } + @Test + void multipleRequestContextPushBeforeLeak() { + final ServiceRequestContext sctx1 = newCtx("/1"); + final ServiceRequestContext sctx2 = newCtx("/2"); + try (SafeCloseable ignore1 = sctx1.push()) { + final ClientRequestContext cctx1 = newClientCtx("/3"); + try (SafeCloseable ignore3 = cctx1.push()) { + assertThatThrownBy(sctx2::push).isInstanceOf(IllegalStateException.class) + .hasMessageContaining("pushed at the following stacktrace"); + } + } + } + + @Test + @SuppressWarnings("MustBeClosedChecker") + void connerCase() { + final AtomicReference exception = new AtomicReference<>(); + + try (DeferredClose deferredClose = new DeferredClose()) { + final ServiceRequestContext ctx = newCtx("/1"); + try (SafeCloseable ignored = ctx.push()) { + final ClientRequestContext ctx2 = newClientCtx("/2"); + ctx2.push(); // <- Leaked, should show in error. + deferredClose.add(ctx2); + final ClientRequestContext ctx3 = newClientCtx("/3"); + try (SafeCloseable ignored1 = ctx3.push()) { + //Ignore + } + } catch (Exception ex) { + exception.set(ex); + } + } + assertThat(exception.get()) + .hasMessageContaining("connerCase(TraceRequestContextLeakTest.java:240)"); + } + private static ServiceRequestContext newCtx(String path) { return ServiceRequestContext.builder(HttpRequest.of(HttpMethod.GET, path)) .build(); } + private static ClientRequestContext newClientCtx(String path) { + return ClientRequestContext.builder(HttpRequest.of(HttpMethod.GET, path)) + .build(); + } + //Utility to clean up RequestContext leak after test private static class DeferredClose implements SafeCloseable { private final ConcurrentHashMap toClose; + private final Set toRemoveFromThreadLocal; DeferredClose() { toClose = new ConcurrentHashMap<>(); + toRemoveFromThreadLocal = new HashSet<>(); } void add(Executor executor, SafeCloseable closeable) { toClose.put(executor, closeable); } + void add(RequestContext requestContext) { + toRemoveFromThreadLocal.add(requestContext); + } + @Override public void close() { toClose.forEach((executor, closeable) -> executor.execute(closeable::close)); + toRemoveFromThreadLocal.forEach(ctx -> RequestContextUtil.pop(ctx, null)); } } } diff --git a/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.FlagsProvider b/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.FlagsProvider index 124763199ff..0c5a2c315a9 100644 --- a/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.FlagsProvider +++ b/it/trace-context-leak/src/test/resources/META-INF/services/com.linecorp.armeria.common.FlagsProvider @@ -1 +1 @@ -com.linecorp.armeria.common.EnableLeakDetectionFlagsProvider +com.linecorp.armeria.internal.common.EnableLeakDetectionFlagsProvider From a72a9cad22d7a096213b8bb37f4cc356936c7e7f Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Sat, 13 Aug 2022 01:44:59 +0700 Subject: [PATCH 27/44] Apply unwrapAll() --- .../armeria/client/ClientRequestContext.java | 2 +- .../armeria/common/RequestContextWrapper.java | 10 --- .../ThreadLocalRequestContextStorage.java | 2 +- .../internal/common/RequestContextUtil.java | 3 +- .../armeria/server/ServiceRequestContext.java | 2 +- .../common/ClientRequestContextTest.java | 87 ++++++++++--------- .../common/ServiceRequestContextTest.java | 71 ++++++++------- 7 files changed, 91 insertions(+), 86 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java b/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java index a79d7507126..a5721aa0473 100644 --- a/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java +++ b/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java @@ -220,7 +220,7 @@ default SafeCloseable push() { return RequestContextUtil.invokeHookAndPop(this, null); } - if (oldCtx.equals(this)) { + if (oldCtx.unwrapAll() == unwrapAll()) { // Reentrance return noopSafeCloseable(); } diff --git a/core/src/main/java/com/linecorp/armeria/common/RequestContextWrapper.java b/core/src/main/java/com/linecorp/armeria/common/RequestContextWrapper.java index f9fb58cc8ae..20c3ae70a75 100644 --- a/core/src/main/java/com/linecorp/armeria/common/RequestContextWrapper.java +++ b/core/src/main/java/com/linecorp/armeria/common/RequestContextWrapper.java @@ -229,14 +229,4 @@ public ExchangeType exchangeType() { public String toString() { return unwrap().toString(); } - - @Override - public boolean equals(Object obj) { - return delegate().equals(obj); - } - - @Override - public int hashCode() { - return delegate().hashCode(); - } } diff --git a/core/src/main/java/com/linecorp/armeria/common/ThreadLocalRequestContextStorage.java b/core/src/main/java/com/linecorp/armeria/common/ThreadLocalRequestContextStorage.java index 3a8239892d1..b4055b7655f 100644 --- a/core/src/main/java/com/linecorp/armeria/common/ThreadLocalRequestContextStorage.java +++ b/core/src/main/java/com/linecorp/armeria/common/ThreadLocalRequestContextStorage.java @@ -46,7 +46,7 @@ public void pop(RequestContext current, @Nullable RequestContext toRestore) { requireNonNull(current, "current"); final InternalThreadLocalMap map = InternalThreadLocalMap.get(); final RequestContext contextInThreadLocal = context.get(map); - if (!contextInThreadLocal.equals(current)) { + if (current.unwrapAll() != contextInThreadLocal.unwrapAll()) { throw newIllegalContextPoppingException(current, contextInThreadLocal); } context.set(map, toRestore); diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java index e85a7176a90..5c22a6f2d40 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java @@ -38,7 +38,8 @@ import com.linecorp.armeria.common.annotation.Nullable; import com.linecorp.armeria.common.util.SafeCloseable; import com.linecorp.armeria.common.util.Sampler; -import com.linecorp.armeria.server.DefaultServiceRequestContext; +import com.linecorp.armeria.internal.client.DefaultClientRequestContext; +import com.linecorp.armeria.internal.server.DefaultServiceRequestContext; import io.netty.channel.ChannelFuture; import io.netty.channel.ChannelFutureListener; diff --git a/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java b/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java index 227ca560df3..43f1a568b1a 100644 --- a/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java +++ b/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java @@ -223,7 +223,7 @@ default SafeCloseable push() { return RequestContextUtil.invokeHookAndPop(this, null); } - if (oldCtx.equals(this)) { + if (oldCtx.unwrapAll() == unwrapAll()) { // Reentrance return noopSafeCloseable(); } diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ClientRequestContextTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ClientRequestContextTest.java index 887e65e631b..dad9f83ea80 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ClientRequestContextTest.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ClientRequestContextTest.java @@ -45,9 +45,9 @@ void current() { final ClientRequestContext ctx = clientRequestContext(); assertThat(ctx.id()).isNotNull(); try (SafeCloseable unused = ctx.push()) { - assertThat(ClientRequestContext.current()).isEqualTo(ctx); + assertThat(ClientRequestContext.current().unwrapAll()).isEqualTo(ctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); try (SafeCloseable unused = serviceRequestContext().push()) { assertThatThrownBy(ClientRequestContext::current) @@ -62,9 +62,9 @@ void currentOrNull() { final ClientRequestContext ctx = clientRequestContext(); try (SafeCloseable unused = ctx.push()) { - assertThat(ClientRequestContext.currentOrNull()).isEqualTo(ctx); + assertThat(ClientRequestContext.currentOrNull().unwrapAll()).isEqualTo(ctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); try (SafeCloseable unused = serviceRequestContext().push()) { assertThat(ClientRequestContext.currentOrNull()).isNull(); @@ -79,9 +79,10 @@ void mapCurrent() { final ClientRequestContext ctx = clientRequestContext(); try (SafeCloseable unused = ctx.push()) { assertThat(ClientRequestContext.mapCurrent(c -> "foo", () -> "bar")).isEqualTo("foo"); - assertThat(ClientRequestContext.mapCurrent(Function.identity(), null)).isEqualTo(ctx); + assertThat(ClientRequestContext.mapCurrent(Function.identity(), null).unwrapAll()) + .isEqualTo(ctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); try (SafeCloseable unused = serviceRequestContext().push()) { assertThatThrownBy(() -> ClientRequestContext.mapCurrent(c -> "foo", () -> "bar")) @@ -94,32 +95,32 @@ void mapCurrent() { void pushReentrance() { final ClientRequestContext ctx = clientRequestContext(); try (SafeCloseable ignored = ctx.push()) { - assertCurrentCtx(ctx); + assertUnwrapAllCurrentCtx(ctx); try (SafeCloseable ignored2 = ctx.push()) { - assertCurrentCtx(ctx); + assertUnwrapAllCurrentCtx(ctx); } - assertCurrentCtx(ctx); + assertUnwrapAllCurrentCtx(ctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); } @Test void pushWithOldServiceCtx() { final ServiceRequestContext sctx = serviceRequestContext(); try (SafeCloseable ignored = sctx.push()) { - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); // The root of ClientRequestContext is sctx. final ClientRequestContext cctx = clientRequestContext(); try (SafeCloseable ignored1 = cctx.push()) { - assertCurrentCtx(cctx); + assertUnwrapAllCurrentCtx(cctx); try (SafeCloseable ignored2 = sctx.push()) { - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); } - assertCurrentCtx(cctx); + assertUnwrapAllCurrentCtx(cctx); } - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); } @Test @@ -139,65 +140,65 @@ void pushWithOldServiceCtx_exceptionWhenServiceCtxIsDifferFromRoot() { void pushWithOldClientCtxWhoseRootIsSameServiceCtx_ctx2IsCreatedSameLayer() { final ServiceRequestContext sctx = serviceRequestContext(); try (SafeCloseable ignored = sctx.push()) { - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); final ClientRequestContext cctx1 = clientRequestContext(); final ClientRequestContext cctx2 = clientRequestContext(); assertThat(cctx1.root()).isEqualTo(cctx2.root()); try (SafeCloseable ignored1 = cctx1.push()) { - assertCurrentCtx(cctx1); + assertUnwrapAllCurrentCtx(cctx1); try (SafeCloseable ignored2 = cctx2.push()) { - assertCurrentCtx(cctx2); + assertUnwrapAllCurrentCtx(cctx2); } - assertCurrentCtx(cctx1); + assertUnwrapAllCurrentCtx(cctx1); } - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); } @Test - void pushWithOldClientCtxWhoseRootIsSameServiceCtx__ctx2IsCreatedUnderCtx1() { + void pushWithOldClientCtxWhoseRootIsSameServiceCtx_ctx2IsCreatedUnderCtx1() { final ServiceRequestContext sctx = serviceRequestContext(); try (SafeCloseable ignored = sctx.push()) { - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); final ClientRequestContext cctx1 = clientRequestContext(); try (SafeCloseable ignored1 = cctx1.push()) { - assertCurrentCtx(cctx1); + assertUnwrapAllCurrentCtx(cctx1); final ClientRequestContext cctx2 = clientRequestContext(); assertThat(cctx1.root()).isEqualTo(cctx2.root()); try (SafeCloseable ignored2 = cctx2.push()) { - assertCurrentCtx(cctx2); + assertUnwrapAllCurrentCtx(cctx2); } - assertCurrentCtx(cctx1); + assertUnwrapAllCurrentCtx(cctx1); } - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); } @Test void pushWithOldClientCtxWhoseRootIsSameServiceCtx_derivedCtx() { final ServiceRequestContext sctx = serviceRequestContext(); try (SafeCloseable ignored = sctx.push()) { - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); final ClientRequestContext cctx1 = clientRequestContext(); final ClientRequestContext derived = cctx1.newDerivedContext(cctx1.id(), cctx1.request(), cctx1.rpcRequest(), cctx1.endpoint()); try (SafeCloseable ignored1 = derived.push()) { - assertCurrentCtx(derived); + assertUnwrapAllCurrentCtx(derived); final ClientRequestContext cctx2 = clientRequestContext(); assertThat(derived.root()).isEqualTo(cctx2.root()); try (SafeCloseable ignored2 = cctx2.push()) { - assertCurrentCtx(cctx2); + assertUnwrapAllCurrentCtx(cctx2); } - assertCurrentCtx(derived); + assertUnwrapAllCurrentCtx(derived); } - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); } @Test @@ -221,16 +222,16 @@ void pushWithOldClientCtxWhoseRootIsDifferent() { void pushWithOldClientCtxWhoseRootIsNull() { final ClientRequestContext cctx1 = clientRequestContext(); try (SafeCloseable ignored1 = cctx1.push()) { - assertCurrentCtx(cctx1); + assertUnwrapAllCurrentCtx(cctx1); final ClientRequestContext cctx2 = clientRequestContext(); assertThat(cctx1.root()).isNull(); assertThat(cctx2.root()).isNull(); try (SafeCloseable ignored2 = cctx2.push()) { - assertCurrentCtx(cctx2); + assertUnwrapAllCurrentCtx(cctx2); } - assertCurrentCtx(cctx1); + assertUnwrapAllCurrentCtx(cctx1); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); } @Test @@ -263,9 +264,13 @@ void hasOwnAttr() { } } - private static void assertCurrentCtx(@Nullable RequestContext ctx) { + private static void assertUnwrapAllCurrentCtx(@Nullable RequestContext ctx) { final RequestContext current = RequestContext.currentOrNull(); - assertThat(current).isEqualTo(ctx); + if (current == null) { + assertThat(ctx).isNull(); + } else { + assertThat(current.unwrapAll()).isEqualTo(ctx); + } } private static ServiceRequestContext serviceRequestContext() { diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ServiceRequestContextTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ServiceRequestContextTest.java index 3b23e4f3637..1c8fe7632d0 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ServiceRequestContextTest.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ServiceRequestContextTest.java @@ -21,6 +21,7 @@ import java.util.function.Function; +import org.assertj.core.api.ObjectAssert; import org.junit.jupiter.api.Test; import com.google.common.collect.ImmutableList; @@ -49,13 +50,13 @@ void current() { assertThat(ServiceRequestContext.current()).isSameAs(sctx); final ClientRequestContext cctx = clientRequestContext("/2"); try (SafeCloseable unused1 = cctx.push()) { - assertThat(ServiceRequestContext.current()).isEqualTo(sctx); - assertThat(ClientRequestContext.current()).isEqualTo(cctx); - assertThat((ClientRequestContext) RequestContext.current()).isEqualTo(cctx); + assertThatUnwrapAll(ServiceRequestContext.current()).isEqualTo(sctx); + assertThatUnwrapAll(ClientRequestContext.current()).isEqualTo(cctx); + assertThatUnwrapAll((ClientRequestContext) RequestContext.current()).isEqualTo(cctx); } - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); try (SafeCloseable unused = clientRequestContext().push()) { assertThatThrownBy(ServiceRequestContext::current) @@ -73,13 +74,13 @@ void currentOrNull() { assertThat(ServiceRequestContext.currentOrNull()).isSameAs(sctx); final ClientRequestContext cctx = clientRequestContext(); try (SafeCloseable unused1 = cctx.push()) { - assertThat(ServiceRequestContext.currentOrNull()).isEqualTo(sctx); - assertThat(ClientRequestContext.current()).isEqualTo(cctx); - assertThat((ClientRequestContext) RequestContext.current()).isEqualTo(cctx); + assertThatUnwrapAll(ServiceRequestContext.currentOrNull()).isEqualTo(sctx); + assertThatUnwrapAll(ClientRequestContext.current()).isEqualTo(cctx); + assertThatUnwrapAll((ClientRequestContext) RequestContext.current()).isEqualTo(cctx); } - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); try (SafeCloseable unused = clientRequestContext().push()) { assertThat(ServiceRequestContext.currentOrNull()).isNull(); @@ -103,16 +104,16 @@ void mapCurrent() { assertThat(ServiceRequestContext.mapCurrent(c -> c == sctx ? "foo" : "bar", () -> "defaultValue")) .isEqualTo("foo"); - assertThat(ClientRequestContext.mapCurrent(c -> c.equals(cctx) ? "baz" : "qux", + assertThat(ClientRequestContext.mapCurrent(c -> c.unwrapAll().equals(cctx) ? "baz" : "qux", () -> "defaultValue")) .isEqualTo("baz"); assertThat(ServiceRequestContext.mapCurrent(Function.identity(), null)).isEqualTo(sctx); - assertThat(ClientRequestContext.mapCurrent(Function.identity(), null)).isEqualTo(cctx); - assertThat(RequestContext.mapCurrent(Function.identity(), null)).isEqualTo(cctx); + assertThatUnwrapAll(ClientRequestContext.mapCurrent(Function.identity(), null)).isEqualTo(cctx); + assertThatUnwrapAll(RequestContext.mapCurrent(Function.identity(), null)).isEqualTo(cctx); } - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); try (SafeCloseable unused = clientRequestContext().push()) { assertThatThrownBy(() -> ServiceRequestContext.mapCurrent(c -> "foo", () -> "bar")) @@ -125,43 +126,43 @@ void mapCurrent() { void pushReentrance() { final ServiceRequestContext ctx = serviceRequestContext(); try (SafeCloseable ignored = ctx.push()) { - assertCurrentCtx(ctx); + assertUnwrapAllCurrentCtx(ctx); try (SafeCloseable ignored2 = ctx.push()) { - assertCurrentCtx(ctx); + assertUnwrapAllCurrentCtx(ctx); } - assertCurrentCtx(ctx); + assertUnwrapAllCurrentCtx(ctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); } @Test void pushWithOldClientCtxWhoseRootIsThisServiceCtx() { final ServiceRequestContext sctx = serviceRequestContext(); try (SafeCloseable ignored = sctx.push()) { - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); // The root of ClientRequestContext is sctx. final ClientRequestContext cctx = clientRequestContext(); try (SafeCloseable ignored1 = cctx.push()) { - assertCurrentCtx(cctx); + assertUnwrapAllCurrentCtx(cctx); try (SafeCloseable ignored2 = sctx.push()) { - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); } - assertCurrentCtx(cctx); + assertUnwrapAllCurrentCtx(cctx); } - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); } @Test void pushWithOldIrrelevantClientCtx() { final ClientRequestContext cctx = clientRequestContext(); try (SafeCloseable ignored = cctx.push()) { - assertCurrentCtx(cctx); + assertUnwrapAllCurrentCtx(cctx); final ServiceRequestContext sctx = serviceRequestContext(); assertThatThrownBy(sctx::push).isInstanceOf(IllegalStateException.class); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); } @Test @@ -169,10 +170,10 @@ void pushWithOldIrrelevantServiceCtx() { final ServiceRequestContext sctx1 = serviceRequestContext(); final ServiceRequestContext sctx2 = serviceRequestContext(); try (SafeCloseable ignored = sctx1.push()) { - assertCurrentCtx(sctx1); + assertUnwrapAllCurrentCtx(sctx1); assertThatThrownBy(sctx2::push).isInstanceOf(IllegalStateException.class); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); } @Test @@ -199,9 +200,17 @@ void queryParams() { assertThat(ctx.queryParams("Not exist")).isEmpty(); } - private static void assertCurrentCtx(@Nullable RequestContext ctx) { + private static void assertUnwrapAllCurrentCtx(@Nullable RequestContext ctx) { final RequestContext current = RequestContext.currentOrNull(); - assertThat(current).isEqualTo(ctx); + if (current == null) { + assertThat(ctx).isNull(); + } else { + assertThatUnwrapAll(current).isEqualTo(ctx); + } + } + + private static ObjectAssert assertThatUnwrapAll(T actual) { + return assertThat(actual.unwrapAll()); } private static ServiceRequestContext serviceRequestContext() { From d43b0529debb0d9e7e4453b0885c2ef3ad9d17db Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Tue, 16 Aug 2022 19:04:08 +0900 Subject: [PATCH 28/44] Update core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java Co-authored-by: minux --- .../internal/common/LeakTracingRequestContextStorage.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java b/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java index 9da172c5601..2af0e112552 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java @@ -49,7 +49,7 @@ final class LeakTracingRequestContextStorage implements RequestContextStorage { * @param sampler the {@link Sampler} that determines whether to retain the stacktrace of the context leaks */ LeakTracingRequestContextStorage(RequestContextStorage delegate, - Sampler sampler) { + Sampler sampler) { this.delegate = requireNonNull(delegate, "delegate"); this.sampler = requireNonNull(sampler, "sampler"); } From 4f8a8f25806e87042724f9616cc8409548f3ff53 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Thu, 25 Aug 2022 14:02:21 +0700 Subject: [PATCH 29/44] Use unwrap for root checking --- .../java/com/linecorp/armeria/client/ClientRequestContext.java | 2 +- .../java/com/linecorp/armeria/server/ServiceRequestContext.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java b/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java index a5721aa0473..a4767ec1124 100644 --- a/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java +++ b/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java @@ -226,7 +226,7 @@ default SafeCloseable push() { } final ServiceRequestContext root = root(); - if (oldCtx.root() == root) { + if (oldCtx.root().unwrapAll() == root.unwrapAll()) { return RequestContextUtil.invokeHookAndPop(this, oldCtx); } diff --git a/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java b/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java index 43f1a568b1a..0ba06701b7c 100644 --- a/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java +++ b/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java @@ -228,7 +228,7 @@ default SafeCloseable push() { return noopSafeCloseable(); } - if (oldCtx.root() == this) { + if (oldCtx.root().unwrapAll() == unwrapAll()) { return RequestContextUtil.invokeHookAndPop(this, oldCtx); } From 6cbcfab6fd8ba18fff8a5d6023f6a33debe08a54 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Thu, 25 Aug 2022 14:11:06 +0700 Subject: [PATCH 30/44] Override unwrap method --- .../internal/common/LeakTracingRequestContextStorage.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java b/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java index 2af0e112552..7ca81a04555 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java @@ -76,6 +76,11 @@ public T currentOrNull() { return delegate.currentOrNull(); } + @Override + public RequestContextStorage unwrap() { + return delegate; + } + private static RequestContextWrapper warpRequestContext(RequestContext ctx) { return ctx instanceof ClientRequestContext ? new TraceableClientRequestContext((ClientRequestContext) ctx) From 89ddc46315bafc68c68cd2bfd32222548e87fd6c Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Thu, 25 Aug 2022 15:47:22 +0700 Subject: [PATCH 31/44] Add space on comment --- .../common/TraceRequestContextLeakTest.java | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/TraceRequestContextLeakTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/TraceRequestContextLeakTest.java index 909afe32623..a85f145a05e 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/TraceRequestContextLeakTest.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/TraceRequestContextLeakTest.java @@ -61,7 +61,7 @@ void singleThreadContextNotLeak() throws InterruptedException { executor.execute(() -> { final ServiceRequestContext ctx = newCtx("/1"); try (SafeCloseable ignore = ctx.push()) { - //Ignore + // Ignore } catch (Exception ex) { isThrown.set(true); } finally { @@ -74,7 +74,7 @@ void singleThreadContextNotLeak() throws InterruptedException { try (SafeCloseable ignore = anotherCtx.push()) { final ClientRequestContext clientCtx = newClientCtx("/3"); try (SafeCloseable ignore2 = clientCtx.push()) { - //Ignore + // Ignore } } catch (Exception ex) { isThrown.set(true); @@ -105,7 +105,7 @@ void singleThreadContextLeak() throws InterruptedException { executor.execute(() -> { final ServiceRequestContext anotherCtx = newCtx("/2"); try (SafeCloseable ignore = anotherCtx.push()) { - //Ignore + // Ignore } catch (Exception ex) { isThrown.set(true); exception.set(ex); @@ -138,15 +138,15 @@ void multiThreadContextLeakNotInterfereOthersEventLoop() throws InterruptedExcep }); ex2.execute(() -> { - //Leak happened on the first eventLoop shouldn't affect 2nd eventLoop when trying to push + // Leak happened on the first eventLoop shouldn't affect 2nd eventLoop when trying to push final ServiceRequestContext anotherCtx = newCtx("/2"); try (SafeCloseable ignore1 = anotherCtx.push()) { final ClientRequestContext cctx = newClientCtx("/3"); try (SafeCloseable ignore2 = cctx.push()) { - //Ignore + // Ignore } } catch (Exception ex) { - //Not suppose to throw exception on the second event loop + // Not suppose to throw exception on the second event loop isThrown.set(true); } finally { latch.countDown(); @@ -192,7 +192,7 @@ void multiThreadContextLeak() throws InterruptedException { ex1.execute(() -> { try (SafeCloseable ignore = anotherCtx3.push()) { - //Ignore + // Ignore } catch (Exception ex) { isThrown.set(true); exception.set(ex); @@ -241,7 +241,7 @@ void connerCase() { deferredClose.add(ctx2); final ClientRequestContext ctx3 = newClientCtx("/3"); try (SafeCloseable ignored1 = ctx3.push()) { - //Ignore + // Ignore } } catch (Exception ex) { exception.set(ex); @@ -261,7 +261,7 @@ private static ClientRequestContext newClientCtx(String path) { .build(); } - //Utility to clean up RequestContext leak after test + // Utility to clean up RequestContext leak after test private static class DeferredClose implements SafeCloseable { private final ConcurrentHashMap toClose; From 6b4b9f9eb2ad15954c4490d5d3c9ef44d770c547 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Thu, 25 Aug 2022 15:59:17 +0700 Subject: [PATCH 32/44] Correct happen-before relationship --- .../armeria/internal/common/TraceRequestContextLeakTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/TraceRequestContextLeakTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/TraceRequestContextLeakTest.java index a85f145a05e..3368052d6c9 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/TraceRequestContextLeakTest.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/TraceRequestContextLeakTest.java @@ -107,8 +107,8 @@ void singleThreadContextLeak() throws InterruptedException { try (SafeCloseable ignore = anotherCtx.push()) { // Ignore } catch (Exception ex) { - isThrown.set(true); exception.set(ex); + isThrown.set(true); } }); @@ -194,8 +194,8 @@ void multiThreadContextLeak() throws InterruptedException { try (SafeCloseable ignore = anotherCtx3.push()) { // Ignore } catch (Exception ex) { - isThrown.set(true); exception.set(ex); + isThrown.set(true); } }); From 083d560ecc8372359813c88987a98c6ab80b0c3e Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Thu, 25 Aug 2022 18:56:38 +0700 Subject: [PATCH 33/44] Add check latch to wait for another thread --- .../armeria/internal/common/TraceRequestContextLeakTest.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/TraceRequestContextLeakTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/TraceRequestContextLeakTest.java index 3368052d6c9..7d35a94d855 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/TraceRequestContextLeakTest.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/TraceRequestContextLeakTest.java @@ -139,6 +139,7 @@ void multiThreadContextLeakNotInterfereOthersEventLoop() throws InterruptedExcep ex2.execute(() -> { // Leak happened on the first eventLoop shouldn't affect 2nd eventLoop when trying to push + await().until(() -> latch.getCount() == 1); final ServiceRequestContext anotherCtx = newCtx("/2"); try (SafeCloseable ignore1 = anotherCtx.push()) { final ClientRequestContext cctx = newClientCtx("/3"); @@ -201,7 +202,7 @@ void multiThreadContextLeak() throws InterruptedException { await().untilTrue(isThrown); assertThat(exception.get()) - .hasMessageContaining("multiThreadContextLeak$6(TraceRequestContextLeakTest.java:178)"); + .hasMessageContaining("multiThreadContextLeak$7(TraceRequestContextLeakTest.java:179)"); } } @@ -248,7 +249,7 @@ void connerCase() { } } assertThat(exception.get()) - .hasMessageContaining("connerCase(TraceRequestContextLeakTest.java:240)"); + .hasMessageContaining("connerCase(TraceRequestContextLeakTest.java:241)"); } private static ServiceRequestContext newCtx(String path) { From 4fb95758bb9497fb9d8e85030ea68a9536a0dc4c Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Sun, 28 Aug 2022 23:36:21 +0700 Subject: [PATCH 34/44] Ensure multiThreadContextLeak executor2 summit --- .../common/TraceRequestContextLeakTest.java | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/TraceRequestContextLeakTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/TraceRequestContextLeakTest.java index 7d35a94d855..82fff976316 100644 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/TraceRequestContextLeakTest.java +++ b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/TraceRequestContextLeakTest.java @@ -164,11 +164,12 @@ void multiThreadContextLeakNotInterfereOthersEventLoop() throws InterruptedExcep void multiThreadContextLeak() throws InterruptedException { final AtomicBoolean isThrown = new AtomicBoolean(false); final AtomicReference exception = new AtomicReference<>(); + final CountDownLatch waitForExecutor2 = new CountDownLatch(1); final EventLoopGroup executor = eventLoopGroupExtension.get(); - final ServiceRequestContext ctx = newCtx("/1-leak"); - final ServiceRequestContext anotherCtx = newCtx("/2-leak"); + final ServiceRequestContext leakingCtx = newCtx("/1-leak"); + final ServiceRequestContext anotherCtx2 = newCtx("/2-leak"); final ServiceRequestContext anotherCtx3 = newCtx("/3-leak"); final Executor ex1 = executor.next(); @@ -176,19 +177,22 @@ void multiThreadContextLeak() throws InterruptedException { try (DeferredClose deferredClose = new DeferredClose()) { ex1.execute(() -> { - final SafeCloseable leaked = ctx.push(); // <- Leaked, should show in error. + final SafeCloseable leaked = leakingCtx.push(); // <- Leaked, should show in error. deferredClose.add(ex1, leaked); }); ex2.execute(() -> { try { - final SafeCloseable leaked = anotherCtx.push(); + final SafeCloseable leaked = anotherCtx2.push(); deferredClose.add(ex2, leaked); } catch (Exception ex) { isThrown.set(true); + } finally { + waitForExecutor2.countDown(); } }); + waitForExecutor2.await(); assertThat(isThrown).isFalse(); ex1.execute(() -> { @@ -202,7 +206,7 @@ void multiThreadContextLeak() throws InterruptedException { await().untilTrue(isThrown); assertThat(exception.get()) - .hasMessageContaining("multiThreadContextLeak$7(TraceRequestContextLeakTest.java:179)"); + .hasMessageContaining("multiThreadContextLeak$7(TraceRequestContextLeakTest.java:180)"); } } @@ -249,7 +253,7 @@ void connerCase() { } } assertThat(exception.get()) - .hasMessageContaining("connerCase(TraceRequestContextLeakTest.java:241)"); + .hasMessageContaining("connerCase(TraceRequestContextLeakTest.java:245)"); } private static ServiceRequestContext newCtx(String path) { From 7b3f3d1e55a88eec4a50d8063aa4b628d6558456 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Mon, 29 Aug 2022 00:56:17 +0700 Subject: [PATCH 35/44] Use StackTraceElement --- .../LeakTracingRequestContextStorage.java | 71 +++++++++---------- 1 file changed, 34 insertions(+), 37 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java b/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java index 7ca81a04555..cf8ed4933b5 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java @@ -19,9 +19,6 @@ import static java.lang.Thread.currentThread; import static java.util.Objects.requireNonNull; -import java.io.PrintWriter; -import java.io.StringWriter; - import com.linecorp.armeria.client.ClientRequestContext; import com.linecorp.armeria.client.ClientRequestContextWrapper; import com.linecorp.armeria.common.RequestContext; @@ -89,51 +86,51 @@ private static RequestContextWrapper warpRequestContext(RequestContext ctx) { private static final class TraceableClientRequestContext extends ClientRequestContextWrapper { - private final PendingRequestContextStackTrace stackTrace; - - private TraceableClientRequestContext(ClientRequestContext delegate) { - super(delegate); - stackTrace = new PendingRequestContextStackTrace(); + private final StackTraceElement[] stackTrace; + private final String threadName; + + private TraceableClientRequestContext(ClientRequestContext delegate) { + super(delegate); + stackTrace = currentThread().getStackTrace(); + threadName = currentThread().getName(); + } + + @Override + public String toString() { + final StringBuilder builder = new StringBuilder(512); + builder.append(getClass().getSimpleName()) + .append(unwrap()).append(System.lineSeparator()) + .append("At thread [").append(threadName) + .append("] previous RequestContext is pushed at the following stacktrace"); + for (int i = 1; i < stackTrace.length; i++) { + builder.append("\tat ").append(stackTrace[i]).append(System.lineSeparator()); + } + return builder.toString(); + } } - @Override - public String toString() { - final StringWriter sw = new StringWriter(); - stackTrace.printStackTrace(new PrintWriter(sw)); - return new StringBuilder().append(getClass().getSimpleName()) - .append(super.toString()) - .append(System.lineSeparator()) - .append(sw).toString(); - } - } - private static final class TraceableServiceRequestContext extends ServiceRequestContextWrapper { - private final PendingRequestContextStackTrace stackTrace; + private final StackTraceElement[] stackTrace; + private final String threadName; private TraceableServiceRequestContext(ServiceRequestContext delegate) { super(delegate); - stackTrace = new PendingRequestContextStackTrace(); + stackTrace = currentThread().getStackTrace(); + threadName = currentThread().getName(); } @Override public String toString() { - final StringWriter sw = new StringWriter(); - stackTrace.printStackTrace(new PrintWriter(sw)); - return new StringBuilder().append(getClass().getSimpleName()) - .append(super.toString()) - .append(System.lineSeparator()) - .append(sw).toString(); - } - } - - private static final class PendingRequestContextStackTrace extends RuntimeException { - - private static final long serialVersionUID = -689451606253441556L; - - private PendingRequestContextStackTrace() { - super("At thread [" + currentThread().getName() + "] previous RequestContext is pushed at " + - "the following stacktrace"); + final StringBuilder builder = new StringBuilder(512); + builder.append(getClass().getSimpleName()) + .append(unwrap()).append(System.lineSeparator()) + .append("At thread [").append(threadName) + .append("] previous RequestContext is pushed at the following stacktrace"); + for (int i = 1; i < stackTrace.length; i++) { + builder.append("\tat ").append(stackTrace[i]).append(System.lineSeparator()); + } + return builder.toString(); } } } From 9d929bf22b957f1fac337f350fdbaaa143d262a6 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Mon, 29 Aug 2022 21:24:50 +0700 Subject: [PATCH 36/44] Fix case root is null --- .../com/linecorp/armeria/client/ClientRequestContext.java | 3 ++- .../armeria/internal/common/RequestContextUtil.java | 6 ++++++ .../com/linecorp/armeria/server/ServiceRequestContext.java | 3 ++- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java b/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java index a4767ec1124..173dcc8633f 100644 --- a/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java +++ b/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java @@ -17,6 +17,7 @@ package com.linecorp.armeria.client; import static com.google.common.base.Preconditions.checkState; +import static com.linecorp.armeria.internal.common.RequestContextUtil.equalUnwrapAllNullable; import static com.linecorp.armeria.internal.common.RequestContextUtil.newIllegalContextPushingException; import static com.linecorp.armeria.internal.common.RequestContextUtil.noopSafeCloseable; import static java.util.Objects.requireNonNull; @@ -226,7 +227,7 @@ default SafeCloseable push() { } final ServiceRequestContext root = root(); - if (oldCtx.root().unwrapAll() == root.unwrapAll()) { + if (equalUnwrapAllNullable(oldCtx.root(), root)) { return RequestContextUtil.invokeHookAndPop(this, oldCtx); } diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java index 5c22a6f2d40..2de2eb443cd 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java @@ -20,6 +20,7 @@ import static java.util.Objects.requireNonNull; import java.util.Collections; +import java.util.Optional; import java.util.Set; import java.util.function.Function; import java.util.function.Supplier; @@ -201,6 +202,11 @@ public static SafeCloseable invokeHookAndPop(RequestContext current, @Nullable R } } + public static boolean equalUnwrapAllNullable(@Nullable RequestContext ctx1, @Nullable RequestContext ctx2) { + return Optional.ofNullable(ctx1).map(RequestContext::unwrapAll).orElse(null) == + Optional.ofNullable(ctx2).map(RequestContext::unwrapAll).orElse(null); + } + @Nullable private static AutoCloseable invokeHook(RequestContext ctx) { final Supplier hook; diff --git a/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java b/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java index 0ba06701b7c..73a79f911bd 100644 --- a/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java +++ b/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java @@ -15,6 +15,7 @@ */ package com.linecorp.armeria.server; +import static com.linecorp.armeria.internal.common.RequestContextUtil.equalUnwrapAllNullable; import static com.linecorp.armeria.internal.common.RequestContextUtil.newIllegalContextPushingException; import static com.linecorp.armeria.internal.common.RequestContextUtil.noopSafeCloseable; import static java.util.Objects.requireNonNull; @@ -228,7 +229,7 @@ default SafeCloseable push() { return noopSafeCloseable(); } - if (oldCtx.root().unwrapAll() == unwrapAll()) { + if (equalUnwrapAllNullable(oldCtx.root(), this)) { return RequestContextUtil.invokeHookAndPop(this, oldCtx); } From 90e6e787aa199d454652b0549f1995c29dcbb3a2 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Mon, 29 Aug 2022 21:37:57 +0700 Subject: [PATCH 37/44] Dedup toString --- .../LeakTracingRequestContextStorage.java | 32 ++++++++----------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java b/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java index cf8ed4933b5..38e98984dc1 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java @@ -84,6 +84,18 @@ private static RequestContextWrapper warpRequestContext(RequestContext ctx) { : new TraceableServiceRequestContext((ServiceRequestContext) ctx); } + private static String stacktraceToString(StackTraceElement[] stackTrace, String threadName, RequestContext unwrap) { + final StringBuilder builder = new StringBuilder(512); + builder.append(unwrap).append(System.lineSeparator()) + .append("At thread [").append(threadName) + .append("] previous RequestContext is pushed at the following stacktrace") + .append(System.lineSeparator()); + for (int i = 1; i < stackTrace.length; i++) { + builder.append("\tat ").append(stackTrace[i]).append(System.lineSeparator()); + } + return builder.toString(); + } + private static final class TraceableClientRequestContext extends ClientRequestContextWrapper { private final StackTraceElement[] stackTrace; @@ -97,15 +109,7 @@ private TraceableClientRequestContext(ClientRequestContext delegate) { @Override public String toString() { - final StringBuilder builder = new StringBuilder(512); - builder.append(getClass().getSimpleName()) - .append(unwrap()).append(System.lineSeparator()) - .append("At thread [").append(threadName) - .append("] previous RequestContext is pushed at the following stacktrace"); - for (int i = 1; i < stackTrace.length; i++) { - builder.append("\tat ").append(stackTrace[i]).append(System.lineSeparator()); - } - return builder.toString(); + return stacktraceToString(stackTrace, threadName, unwrap()); } } @@ -122,15 +126,7 @@ private TraceableServiceRequestContext(ServiceRequestContext delegate) { @Override public String toString() { - final StringBuilder builder = new StringBuilder(512); - builder.append(getClass().getSimpleName()) - .append(unwrap()).append(System.lineSeparator()) - .append("At thread [").append(threadName) - .append("] previous RequestContext is pushed at the following stacktrace"); - for (int i = 1; i < stackTrace.length; i++) { - builder.append("\tat ").append(stackTrace[i]).append(System.lineSeparator()); - } - return builder.toString(); + return stacktraceToString(stackTrace, threadName, unwrap()); } } } From a0540c8f3233fc7f617576a6875b598b75f09e42 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Mon, 29 Aug 2022 23:35:59 +0700 Subject: [PATCH 38/44] Use gradle task to copy test from core --- .../client/ClientRequestContextTest.java | 87 +++--- .../server/ServiceRequestContextTest.java | 71 +++-- it/trace-context-leak/build.gradle | 8 + .../common/ClientRequestContextTest.java | 283 ------------------ .../common/ServiceRequestContextTest.java | 231 -------------- 5 files changed, 94 insertions(+), 586 deletions(-) create mode 100644 it/trace-context-leak/build.gradle delete mode 100644 it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ClientRequestContextTest.java delete mode 100644 it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ServiceRequestContextTest.java diff --git a/core/src/test/java/com/linecorp/armeria/client/ClientRequestContextTest.java b/core/src/test/java/com/linecorp/armeria/client/ClientRequestContextTest.java index b1193e12c68..c89477c4e4e 100644 --- a/core/src/test/java/com/linecorp/armeria/client/ClientRequestContextTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/ClientRequestContextTest.java @@ -41,9 +41,9 @@ void current() { final ClientRequestContext ctx = clientRequestContext(); assertThat(ctx.id()).isNotNull(); try (SafeCloseable unused = ctx.push()) { - assertThat(ClientRequestContext.current()).isSameAs(ctx); + assertThat(ClientRequestContext.current().unwrapAll()).isSameAs(ctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); try (SafeCloseable unused = serviceRequestContext().push()) { assertThatThrownBy(ClientRequestContext::current) @@ -58,9 +58,9 @@ void currentOrNull() { final ClientRequestContext ctx = clientRequestContext(); try (SafeCloseable unused = ctx.push()) { - assertThat(ClientRequestContext.currentOrNull()).isSameAs(ctx); + assertThat(ClientRequestContext.currentOrNull().unwrapAll()).isSameAs(ctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); try (SafeCloseable unused = serviceRequestContext().push()) { assertThat(ClientRequestContext.currentOrNull()).isNull(); @@ -75,9 +75,10 @@ void mapCurrent() { final ClientRequestContext ctx = clientRequestContext(); try (SafeCloseable unused = ctx.push()) { assertThat(ClientRequestContext.mapCurrent(c -> "foo", () -> "bar")).isEqualTo("foo"); - assertThat(ClientRequestContext.mapCurrent(Function.identity(), null)).isSameAs(ctx); + assertThat(ClientRequestContext.mapCurrent(Function.identity(), null).unwrapAll()) + .isSameAs(ctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); try (SafeCloseable unused = serviceRequestContext().push()) { assertThatThrownBy(() -> ClientRequestContext.mapCurrent(c -> "foo", () -> "bar")) @@ -90,32 +91,32 @@ void mapCurrent() { void pushReentrance() { final ClientRequestContext ctx = clientRequestContext(); try (SafeCloseable ignored = ctx.push()) { - assertCurrentCtx(ctx); + assertUnwrapAllCurrentCtx(ctx); try (SafeCloseable ignored2 = ctx.push()) { - assertCurrentCtx(ctx); + assertUnwrapAllCurrentCtx(ctx); } - assertCurrentCtx(ctx); + assertUnwrapAllCurrentCtx(ctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); } @Test void pushWithOldServiceCtx() { final ServiceRequestContext sctx = serviceRequestContext(); try (SafeCloseable ignored = sctx.push()) { - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); // The root of ClientRequestContext is sctx. final ClientRequestContext cctx = clientRequestContext(); try (SafeCloseable ignored1 = cctx.push()) { - assertCurrentCtx(cctx); + assertUnwrapAllCurrentCtx(cctx); try (SafeCloseable ignored2 = sctx.push()) { - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); } - assertCurrentCtx(cctx); + assertUnwrapAllCurrentCtx(cctx); } - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); } @Test @@ -135,65 +136,65 @@ void pushWithOldServiceCtx_exceptionWhenServiceCtxIsDifferFromRoot() { void pushWithOldClientCtxWhoseRootIsSameServiceCtx_ctx2IsCreatedSameLayer() { final ServiceRequestContext sctx = serviceRequestContext(); try (SafeCloseable ignored = sctx.push()) { - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); final ClientRequestContext cctx1 = clientRequestContext(); final ClientRequestContext cctx2 = clientRequestContext(); assertThat(cctx1.root()).isSameAs(cctx2.root()); try (SafeCloseable ignored1 = cctx1.push()) { - assertCurrentCtx(cctx1); + assertUnwrapAllCurrentCtx(cctx1); try (SafeCloseable ignored2 = cctx2.push()) { - assertCurrentCtx(cctx2); + assertUnwrapAllCurrentCtx(cctx2); } - assertCurrentCtx(cctx1); + assertUnwrapAllCurrentCtx(cctx1); } - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); } @Test - void pushWithOldClientCtxWhoseRootIsSameServiceCtx__ctx2IsCreatedUnderCtx1() { + void pushWithOldClientCtxWhoseRootIsSameServiceCtx_ctx2IsCreatedUnderCtx1() { final ServiceRequestContext sctx = serviceRequestContext(); try (SafeCloseable ignored = sctx.push()) { - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); final ClientRequestContext cctx1 = clientRequestContext(); try (SafeCloseable ignored1 = cctx1.push()) { - assertCurrentCtx(cctx1); + assertUnwrapAllCurrentCtx(cctx1); final ClientRequestContext cctx2 = clientRequestContext(); assertThat(cctx1.root()).isSameAs(cctx2.root()); try (SafeCloseable ignored2 = cctx2.push()) { - assertCurrentCtx(cctx2); + assertUnwrapAllCurrentCtx(cctx2); } - assertCurrentCtx(cctx1); + assertUnwrapAllCurrentCtx(cctx1); } - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); } @Test void pushWithOldClientCtxWhoseRootIsSameServiceCtx_derivedCtx() { final ServiceRequestContext sctx = serviceRequestContext(); try (SafeCloseable ignored = sctx.push()) { - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); final ClientRequestContext cctx1 = clientRequestContext(); final ClientRequestContext derived = cctx1.newDerivedContext(cctx1.id(), cctx1.request(), cctx1.rpcRequest(), cctx1.endpoint()); try (SafeCloseable ignored1 = derived.push()) { - assertCurrentCtx(derived); + assertUnwrapAllCurrentCtx(derived); final ClientRequestContext cctx2 = clientRequestContext(); assertThat(derived.root()).isSameAs(cctx2.root()); try (SafeCloseable ignored2 = cctx2.push()) { - assertCurrentCtx(cctx2); + assertUnwrapAllCurrentCtx(cctx2); } - assertCurrentCtx(derived); + assertUnwrapAllCurrentCtx(derived); } - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); } @Test @@ -217,16 +218,16 @@ void pushWithOldClientCtxWhoseRootIsDifferent() { void pushWithOldClientCtxWhoseRootIsNull() { final ClientRequestContext cctx1 = clientRequestContext(); try (SafeCloseable ignored1 = cctx1.push()) { - assertCurrentCtx(cctx1); + assertUnwrapAllCurrentCtx(cctx1); final ClientRequestContext cctx2 = clientRequestContext(); assertThat(cctx1.root()).isNull(); assertThat(cctx2.root()).isNull(); try (SafeCloseable ignored2 = cctx2.push()) { - assertCurrentCtx(cctx2); + assertUnwrapAllCurrentCtx(cctx2); } - assertCurrentCtx(cctx1); + assertUnwrapAllCurrentCtx(cctx1); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); } @Test @@ -259,9 +260,13 @@ void hasOwnAttr() { } } - private static void assertCurrentCtx(@Nullable RequestContext ctx) { + private static void assertUnwrapAllCurrentCtx(@Nullable RequestContext ctx) { final RequestContext current = RequestContext.currentOrNull(); - assertThat(current).isSameAs(ctx); + if (current == null) { + assertThat(ctx).isNull(); + } else { + assertThat(current.unwrapAll()).isSameAs(ctx); + } } private static ServiceRequestContext serviceRequestContext() { diff --git a/core/src/test/java/com/linecorp/armeria/server/ServiceRequestContextTest.java b/core/src/test/java/com/linecorp/armeria/server/ServiceRequestContextTest.java index b4c157cc9ab..927aecee1fd 100644 --- a/core/src/test/java/com/linecorp/armeria/server/ServiceRequestContextTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/ServiceRequestContextTest.java @@ -20,6 +20,7 @@ import java.util.function.Function; +import org.assertj.core.api.ObjectAssert; import org.junit.jupiter.api.Test; import com.google.common.collect.ImmutableList; @@ -44,13 +45,13 @@ void current() { assertThat(ServiceRequestContext.current()).isSameAs(sctx); final ClientRequestContext cctx = clientRequestContext(); try (SafeCloseable unused1 = cctx.push()) { - assertThat(ServiceRequestContext.current()).isSameAs(sctx); - assertThat(ClientRequestContext.current()).isSameAs(cctx); - assertThat((ClientRequestContext) RequestContext.current()).isSameAs(cctx); + assertThatUnwrapAll(ServiceRequestContext.current()).isSameAs(sctx); + assertThatUnwrapAll(ClientRequestContext.current()).isSameAs(cctx); + assertThatUnwrapAll((ClientRequestContext) RequestContext.current()).isSameAs(cctx); } - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); try (SafeCloseable unused = clientRequestContext().push()) { assertThatThrownBy(ServiceRequestContext::current) @@ -68,13 +69,13 @@ void currentOrNull() { assertThat(ServiceRequestContext.currentOrNull()).isSameAs(sctx); final ClientRequestContext cctx = clientRequestContext(); try (SafeCloseable unused1 = cctx.push()) { - assertThat(ServiceRequestContext.currentOrNull()).isSameAs(sctx); - assertThat(ClientRequestContext.current()).isSameAs(cctx); - assertThat((ClientRequestContext) RequestContext.current()).isSameAs(cctx); + assertThatUnwrapAll(ServiceRequestContext.currentOrNull()).isSameAs(sctx); + assertThatUnwrapAll(ClientRequestContext.current()).isSameAs(cctx); + assertThatUnwrapAll((ClientRequestContext) RequestContext.current()).isSameAs(cctx); } - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); try (SafeCloseable unused = clientRequestContext().push()) { assertThat(ServiceRequestContext.currentOrNull()).isNull(); @@ -98,16 +99,16 @@ void mapCurrent() { assertThat(ServiceRequestContext.mapCurrent(c -> c == sctx ? "foo" : "bar", () -> "defaultValue")) .isEqualTo("foo"); - assertThat(ClientRequestContext.mapCurrent(c -> c == cctx ? "baz" : "qux", + assertThat(ClientRequestContext.mapCurrent(c -> c.unwrapAll() == cctx ? "baz" : "qux", () -> "defaultValue")) .isEqualTo("baz"); assertThat(ServiceRequestContext.mapCurrent(Function.identity(), null)).isSameAs(sctx); - assertThat(ClientRequestContext.mapCurrent(Function.identity(), null)).isSameAs(cctx); - assertThat(RequestContext.mapCurrent(Function.identity(), null)).isSameAs(cctx); + assertThatUnwrapAll(ClientRequestContext.mapCurrent(Function.identity(), null)).isSameAs(cctx); + assertThatUnwrapAll(RequestContext.mapCurrent(Function.identity(), null)).isSameAs(cctx); } - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); try (SafeCloseable unused = clientRequestContext().push()) { assertThatThrownBy(() -> ServiceRequestContext.mapCurrent(c -> "foo", () -> "bar")) @@ -120,43 +121,43 @@ void mapCurrent() { void pushReentrance() { final ServiceRequestContext ctx = serviceRequestContext(); try (SafeCloseable ignored = ctx.push()) { - assertCurrentCtx(ctx); + assertUnwrapAllCurrentCtx(ctx); try (SafeCloseable ignored2 = ctx.push()) { - assertCurrentCtx(ctx); + assertUnwrapAllCurrentCtx(ctx); } - assertCurrentCtx(ctx); + assertUnwrapAllCurrentCtx(ctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); } @Test void pushWithOldClientCtxWhoseRootIsThisServiceCtx() { final ServiceRequestContext sctx = serviceRequestContext(); try (SafeCloseable ignored = sctx.push()) { - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); // The root of ClientRequestContext is sctx. final ClientRequestContext cctx = clientRequestContext(); try (SafeCloseable ignored1 = cctx.push()) { - assertCurrentCtx(cctx); + assertUnwrapAllCurrentCtx(cctx); try (SafeCloseable ignored2 = sctx.push()) { - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); } - assertCurrentCtx(cctx); + assertUnwrapAllCurrentCtx(cctx); } - assertCurrentCtx(sctx); + assertUnwrapAllCurrentCtx(sctx); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); } @Test void pushWithOldIrrelevantClientCtx() { final ClientRequestContext cctx = clientRequestContext(); try (SafeCloseable ignored = cctx.push()) { - assertCurrentCtx(cctx); + assertUnwrapAllCurrentCtx(cctx); final ServiceRequestContext sctx = serviceRequestContext(); assertThatThrownBy(sctx::push).isInstanceOf(IllegalStateException.class); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); } @Test @@ -164,10 +165,10 @@ void pushWithOldIrrelevantServiceCtx() { final ServiceRequestContext sctx1 = serviceRequestContext(); final ServiceRequestContext sctx2 = serviceRequestContext(); try (SafeCloseable ignored = sctx1.push()) { - assertCurrentCtx(sctx1); + assertUnwrapAllCurrentCtx(sctx1); assertThatThrownBy(sctx2::push).isInstanceOf(IllegalStateException.class); } - assertCurrentCtx(null); + assertUnwrapAllCurrentCtx(null); } @Test @@ -194,9 +195,17 @@ void queryParams() { assertThat(ctx.queryParams("Not exist")).isEmpty(); } - private static void assertCurrentCtx(@Nullable RequestContext ctx) { + private static void assertUnwrapAllCurrentCtx(@Nullable RequestContext ctx) { final RequestContext current = RequestContext.currentOrNull(); - assertThat(current).isSameAs(ctx); + if (current == null) { + assertThat(ctx).isNull(); + } else { + assertThatUnwrapAll(current).isEqualTo(ctx); + } + } + + private static ObjectAssert assertThatUnwrapAll(T actual) { + return assertThat(actual.unwrapAll()); } private static ServiceRequestContext serviceRequestContext() { diff --git a/it/trace-context-leak/build.gradle b/it/trace-context-leak/build.gradle new file mode 100644 index 00000000000..b3d29626632 --- /dev/null +++ b/it/trace-context-leak/build.gradle @@ -0,0 +1,8 @@ +task generateSources(type: Copy) { + from "${rootProject.projectDir}/core/src/test/java" + into "${project.ext.genSrcDir}/test/java" + include '**/ServiceRequestContextTest.java' + include '**/ClientRequestContextTest.java' +} + +tasks.compileJava.dependsOn(generateSources) \ No newline at end of file diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ClientRequestContextTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ClientRequestContextTest.java deleted file mode 100644 index dad9f83ea80..00000000000 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ClientRequestContextTest.java +++ /dev/null @@ -1,283 +0,0 @@ -/* - * Copyright 2019 LINE Corporation - * - * LINE Corporation licenses this file to you under the Apache License, - * version 2.0 (the "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at: - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT - * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the - * License for the specific language governing permissions and limitations - * under the License. - */ -package com.linecorp.armeria.internal.common; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; - -import java.util.function.Function; - -import org.junit.jupiter.api.Test; - -import com.linecorp.armeria.client.ClientRequestContext; -import com.linecorp.armeria.common.HttpMethod; -import com.linecorp.armeria.common.HttpRequest; -import com.linecorp.armeria.common.RequestContext; -import com.linecorp.armeria.common.annotation.Nullable; -import com.linecorp.armeria.common.util.SafeCloseable; -import com.linecorp.armeria.server.ServiceRequestContext; - -import io.netty.util.AttributeKey; - -/** - * This is copy from ClientRequestContextTest on core module to test behaviour consistency. - */ -class ClientRequestContextTest { - - @Test - void current() { - assertThatThrownBy(ClientRequestContext::current).isInstanceOf(IllegalStateException.class) - .hasMessageContaining("unavailable"); - - final ClientRequestContext ctx = clientRequestContext(); - assertThat(ctx.id()).isNotNull(); - try (SafeCloseable unused = ctx.push()) { - assertThat(ClientRequestContext.current().unwrapAll()).isEqualTo(ctx); - } - assertUnwrapAllCurrentCtx(null); - - try (SafeCloseable unused = serviceRequestContext().push()) { - assertThatThrownBy(ClientRequestContext::current) - .isInstanceOf(IllegalStateException.class) - .hasMessageContaining("not a client-side context"); - } - } - - @Test - void currentOrNull() { - assertThat(ClientRequestContext.currentOrNull()).isNull(); - - final ClientRequestContext ctx = clientRequestContext(); - try (SafeCloseable unused = ctx.push()) { - assertThat(ClientRequestContext.currentOrNull().unwrapAll()).isEqualTo(ctx); - } - assertUnwrapAllCurrentCtx(null); - - try (SafeCloseable unused = serviceRequestContext().push()) { - assertThat(ClientRequestContext.currentOrNull()).isNull(); - } - } - - @Test - void mapCurrent() { - assertThat(ClientRequestContext.mapCurrent(ctx -> "foo", () -> "bar")).isEqualTo("bar"); - assertThat(ClientRequestContext.mapCurrent(Function.identity(), null)).isNull(); - - final ClientRequestContext ctx = clientRequestContext(); - try (SafeCloseable unused = ctx.push()) { - assertThat(ClientRequestContext.mapCurrent(c -> "foo", () -> "bar")).isEqualTo("foo"); - assertThat(ClientRequestContext.mapCurrent(Function.identity(), null).unwrapAll()) - .isEqualTo(ctx); - } - assertUnwrapAllCurrentCtx(null); - - try (SafeCloseable unused = serviceRequestContext().push()) { - assertThatThrownBy(() -> ClientRequestContext.mapCurrent(c -> "foo", () -> "bar")) - .isInstanceOf(IllegalStateException.class) - .hasMessageContaining("not a client-side context"); - } - } - - @Test - void pushReentrance() { - final ClientRequestContext ctx = clientRequestContext(); - try (SafeCloseable ignored = ctx.push()) { - assertUnwrapAllCurrentCtx(ctx); - try (SafeCloseable ignored2 = ctx.push()) { - assertUnwrapAllCurrentCtx(ctx); - } - assertUnwrapAllCurrentCtx(ctx); - } - assertUnwrapAllCurrentCtx(null); - } - - @Test - void pushWithOldServiceCtx() { - final ServiceRequestContext sctx = serviceRequestContext(); - try (SafeCloseable ignored = sctx.push()) { - assertUnwrapAllCurrentCtx(sctx); - // The root of ClientRequestContext is sctx. - final ClientRequestContext cctx = clientRequestContext(); - try (SafeCloseable ignored1 = cctx.push()) { - assertUnwrapAllCurrentCtx(cctx); - try (SafeCloseable ignored2 = sctx.push()) { - assertUnwrapAllCurrentCtx(sctx); - } - assertUnwrapAllCurrentCtx(cctx); - } - assertUnwrapAllCurrentCtx(sctx); - } - assertUnwrapAllCurrentCtx(null); - } - - @Test - void pushWithOldServiceCtx_exceptionWhenServiceCtxIsDifferFromRoot() { - final ServiceRequestContext sctx1 = serviceRequestContext(); - final ClientRequestContext ctx; - try (SafeCloseable ignored = sctx1.push()) { - ctx = clientRequestContext(); - } - final ServiceRequestContext sctx2 = serviceRequestContext(); - try (SafeCloseable ignored = sctx2.push()) { - assertThatThrownBy(ctx::push).isInstanceOf(IllegalStateException.class); - } - } - - @Test - void pushWithOldClientCtxWhoseRootIsSameServiceCtx_ctx2IsCreatedSameLayer() { - final ServiceRequestContext sctx = serviceRequestContext(); - try (SafeCloseable ignored = sctx.push()) { - assertUnwrapAllCurrentCtx(sctx); - final ClientRequestContext cctx1 = clientRequestContext(); - final ClientRequestContext cctx2 = clientRequestContext(); - assertThat(cctx1.root()).isEqualTo(cctx2.root()); - - try (SafeCloseable ignored1 = cctx1.push()) { - assertUnwrapAllCurrentCtx(cctx1); - try (SafeCloseable ignored2 = cctx2.push()) { - assertUnwrapAllCurrentCtx(cctx2); - } - assertUnwrapAllCurrentCtx(cctx1); - } - assertUnwrapAllCurrentCtx(sctx); - } - assertUnwrapAllCurrentCtx(null); - } - - @Test - void pushWithOldClientCtxWhoseRootIsSameServiceCtx_ctx2IsCreatedUnderCtx1() { - final ServiceRequestContext sctx = serviceRequestContext(); - try (SafeCloseable ignored = sctx.push()) { - assertUnwrapAllCurrentCtx(sctx); - final ClientRequestContext cctx1 = clientRequestContext(); - try (SafeCloseable ignored1 = cctx1.push()) { - assertUnwrapAllCurrentCtx(cctx1); - final ClientRequestContext cctx2 = clientRequestContext(); - assertThat(cctx1.root()).isEqualTo(cctx2.root()); - - try (SafeCloseable ignored2 = cctx2.push()) { - assertUnwrapAllCurrentCtx(cctx2); - } - assertUnwrapAllCurrentCtx(cctx1); - } - assertUnwrapAllCurrentCtx(sctx); - } - assertUnwrapAllCurrentCtx(null); - } - - @Test - void pushWithOldClientCtxWhoseRootIsSameServiceCtx_derivedCtx() { - final ServiceRequestContext sctx = serviceRequestContext(); - try (SafeCloseable ignored = sctx.push()) { - assertUnwrapAllCurrentCtx(sctx); - final ClientRequestContext cctx1 = clientRequestContext(); - final ClientRequestContext derived = cctx1.newDerivedContext(cctx1.id(), cctx1.request(), - cctx1.rpcRequest(), cctx1.endpoint()); - try (SafeCloseable ignored1 = derived.push()) { - assertUnwrapAllCurrentCtx(derived); - final ClientRequestContext cctx2 = clientRequestContext(); - assertThat(derived.root()).isEqualTo(cctx2.root()); - - try (SafeCloseable ignored2 = cctx2.push()) { - assertUnwrapAllCurrentCtx(cctx2); - } - assertUnwrapAllCurrentCtx(derived); - } - assertUnwrapAllCurrentCtx(sctx); - } - assertUnwrapAllCurrentCtx(null); - } - - @Test - void pushWithOldClientCtxWhoseRootIsDifferent() { - final ServiceRequestContext sctx1 = serviceRequestContext(); - final ClientRequestContext cctx1; - try (SafeCloseable ignored = sctx1.push()) { - cctx1 = clientRequestContext(); - } - final ServiceRequestContext sctx2 = serviceRequestContext(); - final ClientRequestContext cctx2; - try (SafeCloseable ignored = sctx2.push()) { - cctx2 = clientRequestContext(); - } - try (SafeCloseable ignored = cctx1.push()) { - assertThatThrownBy(cctx2::push).isInstanceOf(IllegalStateException.class); - } - } - - @Test - void pushWithOldClientCtxWhoseRootIsNull() { - final ClientRequestContext cctx1 = clientRequestContext(); - try (SafeCloseable ignored1 = cctx1.push()) { - assertUnwrapAllCurrentCtx(cctx1); - final ClientRequestContext cctx2 = clientRequestContext(); - assertThat(cctx1.root()).isNull(); - assertThat(cctx2.root()).isNull(); - try (SafeCloseable ignored2 = cctx2.push()) { - assertUnwrapAllCurrentCtx(cctx2); - } - assertUnwrapAllCurrentCtx(cctx1); - } - assertUnwrapAllCurrentCtx(null); - } - - @Test - void hasAttr() { - final AttributeKey key = AttributeKey.valueOf(ClientRequestContextTest.class, "KEY"); - final ServiceRequestContext sctx = serviceRequestContext(); - try (SafeCloseable ignored = sctx.push()) { - final ClientRequestContext cctx = clientRequestContext(); - assertThat(sctx.hasAttr(key)).isFalse(); - assertThat(cctx.hasAttr(key)).isFalse(); - - sctx.setAttr(key, "foo"); - assertThat(sctx.hasAttr(key)).isTrue(); - assertThat(cctx.hasAttr(key)).isTrue(); - } - } - - @Test - void hasOwnAttr() { - final AttributeKey key = AttributeKey.valueOf(ClientRequestContextTest.class, "KEY"); - final ServiceRequestContext sctx = serviceRequestContext(); - try (SafeCloseable ignored = sctx.push()) { - final ClientRequestContext cctx = clientRequestContext(); - assertThat(sctx.hasOwnAttr(key)).isFalse(); - assertThat(cctx.hasOwnAttr(key)).isFalse(); - - sctx.setAttr(key, "foo"); - assertThat(sctx.hasOwnAttr(key)).isTrue(); - assertThat(cctx.hasOwnAttr(key)).isFalse(); - } - } - - private static void assertUnwrapAllCurrentCtx(@Nullable RequestContext ctx) { - final RequestContext current = RequestContext.currentOrNull(); - if (current == null) { - assertThat(ctx).isNull(); - } else { - assertThat(current.unwrapAll()).isEqualTo(ctx); - } - } - - private static ServiceRequestContext serviceRequestContext() { - return ServiceRequestContext.of(HttpRequest.of(HttpMethod.GET, "/")); - } - - private static ClientRequestContext clientRequestContext() { - return ClientRequestContext.of(HttpRequest.of(HttpMethod.GET, "/")); - } -} diff --git a/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ServiceRequestContextTest.java b/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ServiceRequestContextTest.java deleted file mode 100644 index 1c8fe7632d0..00000000000 --- a/it/trace-context-leak/src/test/java/com/linecorp/armeria/internal/common/ServiceRequestContextTest.java +++ /dev/null @@ -1,231 +0,0 @@ -/* - * Copyright 2019 LINE Corporation - * - * LINE Corporation licenses this file to you under the Apache License, - * version 2.0 (the "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at: - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT - * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the - * License for the specific language governing permissions and limitations - * under the License. - */ - -package com.linecorp.armeria.internal.common; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; - -import java.util.function.Function; - -import org.assertj.core.api.ObjectAssert; -import org.junit.jupiter.api.Test; - -import com.google.common.collect.ImmutableList; - -import com.linecorp.armeria.client.ClientRequestContext; -import com.linecorp.armeria.common.HttpMethod; -import com.linecorp.armeria.common.HttpRequest; -import com.linecorp.armeria.common.QueryParams; -import com.linecorp.armeria.common.RequestContext; -import com.linecorp.armeria.common.annotation.Nullable; -import com.linecorp.armeria.common.util.SafeCloseable; -import com.linecorp.armeria.server.ServiceRequestContext; - -/** - * This is copy from ServiceRequestContextTest on core module to test behaviour consistency. - */ -class ServiceRequestContextTest { - - @Test - void current() { - assertThatThrownBy(ServiceRequestContext::current).isInstanceOf(IllegalStateException.class) - .hasMessageContaining("unavailable"); - - final ServiceRequestContext sctx = serviceRequestContext("/1"); - try (SafeCloseable unused = sctx.push()) { - assertThat(ServiceRequestContext.current()).isSameAs(sctx); - final ClientRequestContext cctx = clientRequestContext("/2"); - try (SafeCloseable unused1 = cctx.push()) { - assertThatUnwrapAll(ServiceRequestContext.current()).isEqualTo(sctx); - assertThatUnwrapAll(ClientRequestContext.current()).isEqualTo(cctx); - assertThatUnwrapAll((ClientRequestContext) RequestContext.current()).isEqualTo(cctx); - } - assertUnwrapAllCurrentCtx(sctx); - } - assertUnwrapAllCurrentCtx(null); - - try (SafeCloseable unused = clientRequestContext().push()) { - assertThatThrownBy(ServiceRequestContext::current) - .isInstanceOf(IllegalStateException.class) - .hasMessageContaining("not a server-side context"); - } - } - - @Test - void currentOrNull() { - assertThat(ServiceRequestContext.currentOrNull()).isNull(); - - final ServiceRequestContext sctx = serviceRequestContext(); - try (SafeCloseable unused = sctx.push()) { - assertThat(ServiceRequestContext.currentOrNull()).isSameAs(sctx); - final ClientRequestContext cctx = clientRequestContext(); - try (SafeCloseable unused1 = cctx.push()) { - assertThatUnwrapAll(ServiceRequestContext.currentOrNull()).isEqualTo(sctx); - assertThatUnwrapAll(ClientRequestContext.current()).isEqualTo(cctx); - assertThatUnwrapAll((ClientRequestContext) RequestContext.current()).isEqualTo(cctx); - } - assertUnwrapAllCurrentCtx(sctx); - } - assertUnwrapAllCurrentCtx(null); - - try (SafeCloseable unused = clientRequestContext().push()) { - assertThat(ServiceRequestContext.currentOrNull()).isNull(); - } - } - - @Test - void mapCurrent() { - assertThat(ServiceRequestContext.mapCurrent(ctx -> "foo", () -> "defaultValue")) - .isEqualTo("defaultValue"); - assertThat(ServiceRequestContext.mapCurrent(Function.identity(), null)).isNull(); - - final ServiceRequestContext sctx = serviceRequestContext("/1"); - try (SafeCloseable unused = sctx.push()) { - assertThat(ServiceRequestContext.mapCurrent(c -> c == sctx ? "foo" : "bar", - () -> "defaultValue")) - .isEqualTo("foo"); - assertThat(ServiceRequestContext.mapCurrent(Function.identity(), null)).isSameAs(sctx); - final ClientRequestContext cctx = clientRequestContext("/2"); - try (SafeCloseable unused1 = cctx.push()) { - assertThat(ServiceRequestContext.mapCurrent(c -> c == sctx ? "foo" : "bar", - () -> "defaultValue")) - .isEqualTo("foo"); - assertThat(ClientRequestContext.mapCurrent(c -> c.unwrapAll().equals(cctx) ? "baz" : "qux", - () -> "defaultValue")) - .isEqualTo("baz"); - assertThat(ServiceRequestContext.mapCurrent(Function.identity(), null)).isEqualTo(sctx); - assertThatUnwrapAll(ClientRequestContext.mapCurrent(Function.identity(), null)).isEqualTo(cctx); - assertThatUnwrapAll(RequestContext.mapCurrent(Function.identity(), null)).isEqualTo(cctx); - } - assertUnwrapAllCurrentCtx(sctx); - } - assertUnwrapAllCurrentCtx(null); - - try (SafeCloseable unused = clientRequestContext().push()) { - assertThatThrownBy(() -> ServiceRequestContext.mapCurrent(c -> "foo", () -> "bar")) - .isInstanceOf(IllegalStateException.class) - .hasMessageContaining("not a server-side context"); - } - } - - @Test - void pushReentrance() { - final ServiceRequestContext ctx = serviceRequestContext(); - try (SafeCloseable ignored = ctx.push()) { - assertUnwrapAllCurrentCtx(ctx); - try (SafeCloseable ignored2 = ctx.push()) { - assertUnwrapAllCurrentCtx(ctx); - } - assertUnwrapAllCurrentCtx(ctx); - } - assertUnwrapAllCurrentCtx(null); - } - - @Test - void pushWithOldClientCtxWhoseRootIsThisServiceCtx() { - final ServiceRequestContext sctx = serviceRequestContext(); - try (SafeCloseable ignored = sctx.push()) { - assertUnwrapAllCurrentCtx(sctx); - // The root of ClientRequestContext is sctx. - final ClientRequestContext cctx = clientRequestContext(); - try (SafeCloseable ignored1 = cctx.push()) { - assertUnwrapAllCurrentCtx(cctx); - try (SafeCloseable ignored2 = sctx.push()) { - assertUnwrapAllCurrentCtx(sctx); - } - assertUnwrapAllCurrentCtx(cctx); - } - assertUnwrapAllCurrentCtx(sctx); - } - assertUnwrapAllCurrentCtx(null); - } - - @Test - void pushWithOldIrrelevantClientCtx() { - final ClientRequestContext cctx = clientRequestContext(); - try (SafeCloseable ignored = cctx.push()) { - assertUnwrapAllCurrentCtx(cctx); - final ServiceRequestContext sctx = serviceRequestContext(); - assertThatThrownBy(sctx::push).isInstanceOf(IllegalStateException.class); - } - assertUnwrapAllCurrentCtx(null); - } - - @Test - void pushWithOldIrrelevantServiceCtx() { - final ServiceRequestContext sctx1 = serviceRequestContext(); - final ServiceRequestContext sctx2 = serviceRequestContext(); - try (SafeCloseable ignored = sctx1.push()) { - assertUnwrapAllCurrentCtx(sctx1); - assertThatThrownBy(sctx2::push).isInstanceOf(IllegalStateException.class); - } - assertUnwrapAllCurrentCtx(null); - } - - @Test - void queryParams() { - final String path = "/foo"; - final QueryParams queryParams = QueryParams.of("param1", "value1", - "param1", "value2", - "Param1", "Value3", - "PARAM1", "VALUE4"); - final String pathAndQuery = path + '?' + queryParams.toQueryString(); - final ServiceRequestContext ctx = ServiceRequestContext.of(HttpRequest.of(HttpMethod.GET, - pathAndQuery)); - - assertThat(ctx.queryParams()).isEqualTo(queryParams); - - assertThat(ctx.queryParam("param1")).isEqualTo("value1"); - assertThat(ctx.queryParam("Param1")).isEqualTo("Value3"); - assertThat(ctx.queryParam("PARAM1")).isEqualTo("VALUE4"); - assertThat(ctx.queryParam("Not exist")).isNull(); - - assertThat(ctx.queryParams("param1")).isEqualTo(ImmutableList.of("value1", "value2")); - assertThat(ctx.queryParams("Param1")).isEqualTo(ImmutableList.of("Value3")); - assertThat(ctx.queryParams("PARAM1")).isEqualTo(ImmutableList.of("VALUE4")); - assertThat(ctx.queryParams("Not exist")).isEmpty(); - } - - private static void assertUnwrapAllCurrentCtx(@Nullable RequestContext ctx) { - final RequestContext current = RequestContext.currentOrNull(); - if (current == null) { - assertThat(ctx).isNull(); - } else { - assertThatUnwrapAll(current).isEqualTo(ctx); - } - } - - private static ObjectAssert assertThatUnwrapAll(T actual) { - return assertThat(actual.unwrapAll()); - } - - private static ServiceRequestContext serviceRequestContext() { - return serviceRequestContext("/"); - } - - private static ServiceRequestContext serviceRequestContext(String path) { - return ServiceRequestContext.of(HttpRequest.of(HttpMethod.GET, path)); - } - - private static ClientRequestContext clientRequestContext() { - return clientRequestContext("/"); - } - - private static ClientRequestContext clientRequestContext(String path) { - return ClientRequestContext.of(HttpRequest.of(HttpMethod.GET, path)); - } -} From 91568987b073cf63477e2946ec8cb42efd6b6a10 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Mon, 29 Aug 2022 23:56:36 +0700 Subject: [PATCH 39/44] Fix lint --- .../internal/common/LeakTracingRequestContextStorage.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java b/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java index 38e98984dc1..d0e925fcc39 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java @@ -84,7 +84,8 @@ private static RequestContextWrapper warpRequestContext(RequestContext ctx) { : new TraceableServiceRequestContext((ServiceRequestContext) ctx); } - private static String stacktraceToString(StackTraceElement[] stackTrace, String threadName, RequestContext unwrap) { + private static String stacktraceToString(StackTraceElement[] stackTrace, String threadName, + RequestContext unwrap) { final StringBuilder builder = new StringBuilder(512); builder.append(unwrap).append(System.lineSeparator()) .append("At thread [").append(threadName) From 50384a23f1612b1ab21667a7c94ab9f5983f9757 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Tue, 30 Aug 2022 19:57:21 +0700 Subject: [PATCH 40/44] Refactor equalUnwrapAllNullable --- .../armeria/client/ClientRequestContext.java | 4 ++-- .../armeria/internal/common/RequestContextUtil.java | 12 ++++++++---- .../armeria/server/ServiceRequestContext.java | 4 ++-- it/trace-context-leak/build.gradle | 2 +- 4 files changed, 13 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java b/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java index 173dcc8633f..45d21becc3d 100644 --- a/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java +++ b/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java @@ -17,7 +17,7 @@ package com.linecorp.armeria.client; import static com.google.common.base.Preconditions.checkState; -import static com.linecorp.armeria.internal.common.RequestContextUtil.equalUnwrapAllNullable; +import static com.linecorp.armeria.internal.common.RequestContextUtil.equalsUnwrapping; import static com.linecorp.armeria.internal.common.RequestContextUtil.newIllegalContextPushingException; import static com.linecorp.armeria.internal.common.RequestContextUtil.noopSafeCloseable; import static java.util.Objects.requireNonNull; @@ -227,7 +227,7 @@ default SafeCloseable push() { } final ServiceRequestContext root = root(); - if (equalUnwrapAllNullable(oldCtx.root(), root)) { + if (equalsUnwrapping(oldCtx.root(), root)) { return RequestContextUtil.invokeHookAndPop(this, oldCtx); } diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java index 2de2eb443cd..ad45d0fcc9a 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java @@ -20,7 +20,6 @@ import static java.util.Objects.requireNonNull; import java.util.Collections; -import java.util.Optional; import java.util.Set; import java.util.function.Function; import java.util.function.Supplier; @@ -202,9 +201,14 @@ public static SafeCloseable invokeHookAndPop(RequestContext current, @Nullable R } } - public static boolean equalUnwrapAllNullable(@Nullable RequestContext ctx1, @Nullable RequestContext ctx2) { - return Optional.ofNullable(ctx1).map(RequestContext::unwrapAll).orElse(null) == - Optional.ofNullable(ctx2).map(RequestContext::unwrapAll).orElse(null); + public static boolean equalsUnwrapping(@Nullable RequestContext ctx1, @Nullable RequestContext ctx2) { + if (ctx1 == ctx2) { + return true; + } + if (ctx1 != null && ctx2 != null) { + return ctx1.unwrapAll() == ctx2.unwrapAll(); + } + return false; } @Nullable diff --git a/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java b/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java index 73a79f911bd..cea27ae853c 100644 --- a/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java +++ b/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java @@ -15,7 +15,7 @@ */ package com.linecorp.armeria.server; -import static com.linecorp.armeria.internal.common.RequestContextUtil.equalUnwrapAllNullable; +import static com.linecorp.armeria.internal.common.RequestContextUtil.equalsUnwrapping; import static com.linecorp.armeria.internal.common.RequestContextUtil.newIllegalContextPushingException; import static com.linecorp.armeria.internal.common.RequestContextUtil.noopSafeCloseable; import static java.util.Objects.requireNonNull; @@ -229,7 +229,7 @@ default SafeCloseable push() { return noopSafeCloseable(); } - if (equalUnwrapAllNullable(oldCtx.root(), this)) { + if (equalsUnwrapping(oldCtx.root(), this)) { return RequestContextUtil.invokeHookAndPop(this, oldCtx); } diff --git a/it/trace-context-leak/build.gradle b/it/trace-context-leak/build.gradle index b3d29626632..d083ea187dc 100644 --- a/it/trace-context-leak/build.gradle +++ b/it/trace-context-leak/build.gradle @@ -5,4 +5,4 @@ task generateSources(type: Copy) { include '**/ClientRequestContextTest.java' } -tasks.compileJava.dependsOn(generateSources) \ No newline at end of file +tasks.compileJava.dependsOn(generateSources) From 0fe21fd1ad50f983e2777216d18121586381518d Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Mon, 5 Sep 2022 00:49:11 +0700 Subject: [PATCH 41/44] Remove thread name from stacktrace --- .../common/LeakTracingRequestContextStorage.java | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java b/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java index d0e925fcc39..b2075c2277b 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java @@ -84,12 +84,11 @@ private static RequestContextWrapper warpRequestContext(RequestContext ctx) { : new TraceableServiceRequestContext((ServiceRequestContext) ctx); } - private static String stacktraceToString(StackTraceElement[] stackTrace, String threadName, + private static String stacktraceToString(StackTraceElement[] stackTrace, RequestContext unwrap) { final StringBuilder builder = new StringBuilder(512); builder.append(unwrap).append(System.lineSeparator()) - .append("At thread [").append(threadName) - .append("] previous RequestContext is pushed at the following stacktrace") + .append("The previous RequestContext is pushed at the following stacktrace") .append(System.lineSeparator()); for (int i = 1; i < stackTrace.length; i++) { builder.append("\tat ").append(stackTrace[i]).append(System.lineSeparator()); @@ -100,34 +99,30 @@ private static String stacktraceToString(StackTraceElement[] stackTrace, String private static final class TraceableClientRequestContext extends ClientRequestContextWrapper { private final StackTraceElement[] stackTrace; - private final String threadName; private TraceableClientRequestContext(ClientRequestContext delegate) { super(delegate); stackTrace = currentThread().getStackTrace(); - threadName = currentThread().getName(); } @Override public String toString() { - return stacktraceToString(stackTrace, threadName, unwrap()); + return stacktraceToString(stackTrace, unwrap()); } } private static final class TraceableServiceRequestContext extends ServiceRequestContextWrapper { private final StackTraceElement[] stackTrace; - private final String threadName; private TraceableServiceRequestContext(ServiceRequestContext delegate) { super(delegate); stackTrace = currentThread().getStackTrace(); - threadName = currentThread().getName(); } @Override public String toString() { - return stacktraceToString(stackTrace, threadName, unwrap()); + return stacktraceToString(stackTrace, unwrap()); } } } From caba3d61ce6732be977301507cbaa11677f3303d Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Mon, 5 Sep 2022 01:03:24 +0700 Subject: [PATCH 42/44] Refactor equalUnwrapAllNullable --- .../armeria/client/ClientRequestContext.java | 3 +-- .../armeria/common/util/Unwrappable.java | 16 ++++++++++++++++ .../internal/common/RequestContextUtil.java | 11 ++++------- .../armeria/server/ServiceRequestContext.java | 3 +-- 4 files changed, 22 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java b/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java index 45d21becc3d..af277a0f9e8 100644 --- a/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java +++ b/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java @@ -17,7 +17,6 @@ package com.linecorp.armeria.client; import static com.google.common.base.Preconditions.checkState; -import static com.linecorp.armeria.internal.common.RequestContextUtil.equalsUnwrapping; import static com.linecorp.armeria.internal.common.RequestContextUtil.newIllegalContextPushingException; import static com.linecorp.armeria.internal.common.RequestContextUtil.noopSafeCloseable; import static java.util.Objects.requireNonNull; @@ -227,7 +226,7 @@ default SafeCloseable push() { } final ServiceRequestContext root = root(); - if (equalsUnwrapping(oldCtx.root(), root)) { + if (RequestContextUtil.equalsIgnoreWrapper(oldCtx.root(), root)) { return RequestContextUtil.invokeHookAndPop(this, oldCtx); } diff --git a/core/src/main/java/com/linecorp/armeria/common/util/Unwrappable.java b/core/src/main/java/com/linecorp/armeria/common/util/Unwrappable.java index 7244970ef3d..250ea693e22 100644 --- a/core/src/main/java/com/linecorp/armeria/common/util/Unwrappable.java +++ b/core/src/main/java/com/linecorp/armeria/common/util/Unwrappable.java @@ -129,4 +129,20 @@ default Object unwrapAll() { unwrapped = inner; } } + + /** + * Reference checking this {@link Unwrappable} to another {@link Unwrappable}, ignoring wrappers. + * Two {@link Unwrappable} are considered equal ignoring wrappers if they are of the same object reference + * after {@link #unwrapAll()}. + * @param other The {@link Unwrappable} to compare this {@link Unwrappable} against + * @return true if the argument is not {@code null}, and it represents a same object reference after + * {@code unwrapAll()}, false otherwise. + */ + @UnstableApi + default boolean equalsIgnoreWrapper(@Nullable Unwrappable other) { + if (other == null) { + return false; + } + return unwrapAll() == other.unwrapAll(); + } } diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java index ad45d0fcc9a..bd6dc307722 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java @@ -201,14 +201,11 @@ public static SafeCloseable invokeHookAndPop(RequestContext current, @Nullable R } } - public static boolean equalsUnwrapping(@Nullable RequestContext ctx1, @Nullable RequestContext ctx2) { - if (ctx1 == ctx2) { - return true; + public static boolean equalsIgnoreWrapper(@Nullable RequestContext ctx1, @Nullable RequestContext ctx2) { + if (ctx1 == null) { + return ctx2 == null; } - if (ctx1 != null && ctx2 != null) { - return ctx1.unwrapAll() == ctx2.unwrapAll(); - } - return false; + return ctx1.equalsIgnoreWrapper(ctx2); } @Nullable diff --git a/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java b/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java index cea27ae853c..ec1b3e8b43d 100644 --- a/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java +++ b/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java @@ -15,7 +15,6 @@ */ package com.linecorp.armeria.server; -import static com.linecorp.armeria.internal.common.RequestContextUtil.equalsUnwrapping; import static com.linecorp.armeria.internal.common.RequestContextUtil.newIllegalContextPushingException; import static com.linecorp.armeria.internal.common.RequestContextUtil.noopSafeCloseable; import static java.util.Objects.requireNonNull; @@ -229,7 +228,7 @@ default SafeCloseable push() { return noopSafeCloseable(); } - if (equalsUnwrapping(oldCtx.root(), this)) { + if (RequestContextUtil.equalsIgnoreWrapper(oldCtx.root(), this)) { return RequestContextUtil.invokeHookAndPop(this, oldCtx); } From 1302481d3d91b0b573a69ae3e0b0f2617bcafcd5 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Mon, 5 Sep 2022 01:38:19 +0700 Subject: [PATCH 43/44] Add guard clause --- .../armeria/common/ThreadLocalRequestContextStorage.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/ThreadLocalRequestContextStorage.java b/core/src/main/java/com/linecorp/armeria/common/ThreadLocalRequestContextStorage.java index b4055b7655f..cc110929c89 100644 --- a/core/src/main/java/com/linecorp/armeria/common/ThreadLocalRequestContextStorage.java +++ b/core/src/main/java/com/linecorp/armeria/common/ThreadLocalRequestContextStorage.java @@ -46,7 +46,7 @@ public void pop(RequestContext current, @Nullable RequestContext toRestore) { requireNonNull(current, "current"); final InternalThreadLocalMap map = InternalThreadLocalMap.get(); final RequestContext contextInThreadLocal = context.get(map); - if (current.unwrapAll() != contextInThreadLocal.unwrapAll()) { + if (contextInThreadLocal == null || current.unwrapAll() != contextInThreadLocal.unwrapAll()) { throw newIllegalContextPoppingException(current, contextInThreadLocal); } context.set(map, toRestore); From 9e8e10eaa8e10fd0fa100bd7e6df9fa540e74476 Mon Sep 17 00:00:00 2001 From: waritboonmasiri Date: Mon, 5 Sep 2022 19:59:04 +0700 Subject: [PATCH 44/44] Correct spelling and remove UnstableApi --- .../internal/common/LeakTracingRequestContextStorage.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java b/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java index b2075c2277b..752b8c896da 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java @@ -25,7 +25,6 @@ import com.linecorp.armeria.common.RequestContextStorage; import com.linecorp.armeria.common.RequestContextWrapper; import com.linecorp.armeria.common.annotation.Nullable; -import com.linecorp.armeria.common.annotation.UnstableApi; import com.linecorp.armeria.common.util.Sampler; import com.linecorp.armeria.server.ServiceRequestContext; import com.linecorp.armeria.server.ServiceRequestContextWrapper; @@ -34,7 +33,6 @@ * A {@link RequestContextStorage} which keeps track of {@link RequestContext}s, reporting pushed thread * information if a {@link RequestContext} is leaked. */ -@UnstableApi final class LeakTracingRequestContextStorage implements RequestContextStorage { private final RequestContextStorage delegate; @@ -56,7 +54,7 @@ final class LeakTracingRequestContextStorage implements RequestContextStorage { public T push(RequestContext toPush) { requireNonNull(toPush, "toPush"); if (sampler.isSampled(toPush)) { - return delegate.push(warpRequestContext(toPush)); + return delegate.push(wrapRequestContext(toPush)); } return delegate.push(toPush); } @@ -78,7 +76,7 @@ public RequestContextStorage unwrap() { return delegate; } - private static RequestContextWrapper warpRequestContext(RequestContext ctx) { + private static RequestContextWrapper wrapRequestContext(RequestContext ctx) { return ctx instanceof ClientRequestContext ? new TraceableClientRequestContext((ClientRequestContext) ctx) : new TraceableServiceRequestContext((ServiceRequestContext) ctx);