Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TraceAbleRequestContextStorage for ease of detect context leaks #4232

Merged
merged 46 commits into from Sep 5, 2022
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
1d64827
Add TraceAbleRequestContextStorage for ease of detect context leak
klurpicolo Apr 26, 2022
c79b852
Update classname
klurpicolo Apr 28, 2022
443e740
Add Sampler to constructor of LeakTracingRequestContextStorage
klurpicolo Apr 28, 2022
c431103
Add benchmark on LeakTracingRequestContextStorage
klurpicolo May 1, 2022
06c728e
Add finally when pop
klurpicolo May 5, 2022
0f5e5e5
Change generic of Sampler
klurpicolo May 5, 2022
bcf92df
Merge branch 'master' into context-leak-debug
ikhoon May 13, 2022
82bb1a8
Add UnstableApi and check null input
klurpicolo May 13, 2022
a7fd563
Refactor DeferredClose
klurpicolo May 13, 2022
ea2e675
Add current stacktrace to exception
klurpicolo May 14, 2022
cb8dcf4
Use await instead of CountDownLatch in some tests
klurpicolo May 14, 2022
50d4127
Fix lint
klurpicolo May 14, 2022
818c166
Fix test
klurpicolo May 14, 2022
5207a4c
Provide a flag for enable requestContextLeakDetection
klurpicolo May 24, 2022
d24e0a2
Move LeakDetectionConfiguration package outside of internal
klurpicolo May 24, 2022
61e767d
Replace LeakDetectionConfiguration with Sampler<? super RequestContext>
klurpicolo Jun 1, 2022
449f051
Change flag name from requestContextLeakDetection to requestContextLe…
klurpicolo Jun 1, 2022
4dabd01
Refactor RequestContextUtil
klurpicolo Jun 1, 2022
8909ace
Correct Java doc grammar
klurpicolo Jun 1, 2022
fdd8490
Add newline to EOF
klurpicolo Jun 1, 2022
5562cfe
Correct comment and add UnstableApi
klurpicolo Jun 2, 2022
f124fc8
Update generic of Sampler
klurpicolo Jun 3, 2022
a71a725
Move LeakTracingRequestContextStorage to internal.common
klurpicolo Jun 3, 2022
c4a534f
Correct spelling
klurpicolo Jun 3, 2022
3b00c2a
Modifying Samplers.of(String) to accept true and false
klurpicolo Jun 12, 2022
af31a6e
Add illegallyPushServiceRequestContext test
klurpicolo Jun 18, 2022
ab4de84
Refactor LeakTracingRequestContextStorage
klurpicolo Jul 1, 2022
9f681d4
Merge branch 'master' into context-leak-debug
klurpicolo Aug 12, 2022
a72a9ca
Apply unwrapAll()
klurpicolo Aug 12, 2022
d43b052
Update core/src/main/java/com/linecorp/armeria/internal/common/LeakTr…
ikhoon Aug 16, 2022
4f8a8f2
Use unwrap for root checking
klurpicolo Aug 25, 2022
6cbcfab
Override unwrap method
klurpicolo Aug 25, 2022
89ddc46
Add space on comment
klurpicolo Aug 25, 2022
6b4b9f9
Correct happen-before relationship
klurpicolo Aug 25, 2022
083d560
Add check latch to wait for another thread
klurpicolo Aug 25, 2022
4fb9575
Ensure multiThreadContextLeak executor2 summit
klurpicolo Aug 28, 2022
7b3f3d1
Use StackTraceElement
klurpicolo Aug 28, 2022
9d929bf
Fix case root is null
klurpicolo Aug 29, 2022
90e6e78
Dedup toString
klurpicolo Aug 29, 2022
a0540c8
Use gradle task to copy test from core
klurpicolo Aug 29, 2022
9156898
Fix lint
klurpicolo Aug 29, 2022
50384a2
Refactor equalUnwrapAllNullable
klurpicolo Aug 30, 2022
0fe21fd
Remove thread name from stacktrace
klurpicolo Sep 4, 2022
caba3d6
Refactor equalUnwrapAllNullable
klurpicolo Sep 4, 2022
1302481
Add guard clause
klurpicolo Sep 4, 2022
9e8e10e
Correct spelling and remove UnstableApi
klurpicolo Sep 5, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -0,0 +1,95 @@
/*
* 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 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;

/**
* 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);
}
}
Expand Up @@ -216,15 +216,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.unwrapAll() == unwrapAll()) {
// Reentrance
return noopSafeCloseable();
}

final ServiceRequestContext root = root();
if (oldCtx.root() == root) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we also use unwrapAll?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should.

return RequestContextUtil.invokeHookAndPop(this, oldCtx);
Expand Down
Expand Up @@ -398,4 +398,9 @@ public Path defaultMultipartUploadsLocation() {
File.separatorChar + "armeria" +
File.separatorChar + "multipart-uploads");
}

@Override
public Sampler<? super RequestContext> requestContextLeakDetectionSampler() {
return Sampler.never();
}
}
23 changes: 19 additions & 4 deletions core/src/main/java/com/linecorp/armeria/common/Flags.java
Expand Up @@ -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;
Expand Down Expand Up @@ -113,10 +114,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;
Expand Down Expand Up @@ -377,6 +374,9 @@ public final class Flags {
private static final Path DEFAULT_MULTIPART_UPLOADS_LOCATION =
getValue(FlagsProvider::defaultMultipartUploadsLocation, "defaultMultipartUploadsLocation");

private static final Sampler<? super RequestContext> REQUEST_CONTEXT_LEAK_DETECTION_SAMPLER =
getValue(FlagsProvider::requestContextLeakDetectionSampler, "requestContextLeakDetectionSampler");

/**
* 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
Expand Down Expand Up @@ -1296,6 +1296,21 @@ public static boolean allowDoubleDotsInQueryString() {
return ALLOW_DOUBLE_DOTS_IN_QUERY_STRING;
}

/**
* 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.
*
* <p>The default value of this flag is {@link Sampler#never()}.
* Specify the {@code -Dcom.linecorp.armeria.requestContextLeakDetectionSampler=<specification>} JVM option
* to override the default. This feature is disabled if {@link Sampler#never()} is specified.
* See {@link Sampler#of(String)} for the specification string format.</p>
*/
@UnstableApi
public static Sampler<? super RequestContext> requestContextLeakDetectionSampler() {
ikhoon marked this conversation as resolved.
Show resolved Hide resolved
return REQUEST_CONTEXT_LEAK_DETECTION_SAMPLER;
}

@Nullable
private static String nullableCaffeineSpec(Function<FlagsProvider, String> method, String flagName) {
return caffeineSpec(method, flagName, true);
Expand Down
16 changes: 16 additions & 0 deletions core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java
Expand Up @@ -960,4 +960,20 @@ default Boolean allowDoubleDotsInQueryString() {
default Path defaultMultipartUploadsLocation() {
return null;
}

/**
* 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.
*
* <p>The default value of this flag is {@link Sampler#never()}.
* Specify the {@code -Dcom.linecorp.armeria.requestContextLeakDetectionSampler=<specification>} JVM option
* to override the default. This feature is disabled if {@link Sampler#never()} is specified.
* See {@link Sampler#of(String)} for the specification string format.</p>
*/
@UnstableApi
@Nullable
ikhoon marked this conversation as resolved.
Show resolved Hide resolved
default Sampler<? super RequestContext> requestContextLeakDetectionSampler() {
return null;
}
}
Expand Up @@ -71,20 +71,12 @@ public Sampler<Class<? extends Throwable>> 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);
}

Expand Down Expand Up @@ -438,6 +430,20 @@ public Path defaultMultipartUploadsLocation() {
return getAndParse("defaultMultipartUploadsLocation", Paths::get);
}

@Override
public Sampler<? super RequestContext> requestContextLeakDetectionSampler() {
final String spec = getNormalized("requestContextLeakDetectionSampler");
if (spec == null) {
return null;
}
try {
return 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);
Expand Down
Expand Up @@ -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 (current.unwrapAll() != contextInThreadLocal.unwrapAll()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also check if contextInThreadLocal is null or not just in case pop is called without push.

throw newIllegalContextPoppingException(current, contextInThreadLocal);
}
context.set(map, toRestore);
Expand Down
Expand Up @@ -70,8 +70,10 @@ static <T> Sampler<T> of(String specification) {
requireNonNull(specification, "specification");
switch (specification.trim()) {
case "always":
case "true":
return Sampler.always();
case "never":
case "false":
return Sampler.never();
}

Expand Down
@@ -0,0 +1,134 @@
/*
* 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.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 com.linecorp.armeria.server.ServiceRequestContextWrapper;

/**
* A {@link RequestContextStorage} which keeps track of {@link RequestContext}s, reporting pushed thread
* information if a {@link RequestContext} is leaked.
*/
@UnstableApi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this class isn't part of the public api

Suggested change
@UnstableApi

final class LeakTracingRequestContextStorage implements RequestContextStorage {
minwoox marked this conversation as resolved.
Show resolved Hide resolved

private final RequestContextStorage delegate;
private final Sampler<? super RequestContext> sampler;

/**
* 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
*/
LeakTracingRequestContextStorage(RequestContextStorage delegate,
Sampler<? super RequestContext> sampler) {
ikhoon marked this conversation as resolved.
Show resolved Hide resolved
this.delegate = requireNonNull(delegate, "delegate");
this.sampler = requireNonNull(sampler, "sampler");
}

@Nullable
@Override
public <T extends RequestContext> T push(RequestContext toPush) {
requireNonNull(toPush, "toPush");
if (sampler.isSampled(toPush)) {
return delegate.push(warpRequestContext(toPush));
}
return delegate.push(toPush);
}

@Override
public void pop(RequestContext current, @Nullable RequestContext toRestore) {
requireNonNull(current, "current");
delegate.pop(current, toRestore);
}

@Nullable
@Override
public <T extends RequestContext> T currentOrNull() {
return delegate.currentOrNull();
}

private static RequestContextWrapper<?> warpRequestContext(RequestContext ctx) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private static RequestContextWrapper<?> warpRequestContext(RequestContext ctx) {
private static RequestContextWrapper<?> wrapRequestContext(RequestContext ctx) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! misspelled. Thanks 🙏

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();
Copy link
Member

@minwoox minwoox Aug 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using StackTraceElement[] because

  • an exception is not raised yet (we don't need to create an instance of an exception that might not be raised in the future.)
  • all we need in toString is the stack trace?

If we using StackTraceElement[], we could probably do:

private TraceableClientRequestContext(ServiceRequestContext delegate) {
    super(delegate);
    stackTrace = currentThread().getStackTrace();
}

@Override
public String toString() {
    final StringBuilder builder = new StringBuilder(10000); // What's the sensible default?
    builder.append(super.toString());
    builder.append(System.lineSeparator());
    // Start with 1 not to print getStackTrace() method.
    for (int i = 1; i < stackTrace.length; i++) {
        builder.append("\tat ").append(stackTrace[i]).append(System.lineSeparator());
    }
    return builder.toString();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JFYI, StackTraceWriter uses 512 for the initial buffer.

private static final class StackTraceWriter extends PrintWriter {
StackTraceWriter() {
super(new StringWriter(512));

}

@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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dedup?

}
}

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");
}
}
}