Add TraceAbleRequestContextStorage for ease of detect context leaks #4232
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
klurpicolo
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
Codecov Report
@@ Coverage Diff @@
## master #4232 +/- ##
============================================
+ Coverage 73.82% 73.92% +0.09%
- Complexity 17698 17797 +99
============================================
Files 1505 1510 +5
Lines 66105 66262 +157
Branches 8309 8321 +12
============================================
+ Hits 48805 48981 +176
+ Misses 13302 13275 -27
- Partials 3998 4006 +8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
ikhoon
reviewed
Apr 28, 2022
| throw pendingRequestCtx.get(); | ||
| } | ||
| } | ||
| pendingRequestCtx.set(new PendingRequestContextStackTrace(toPush)); |
There was a problem hiding this comment.
Should we take a Sampler as a constructor parameter so that users can customize the sampling rate?
If not specified, I prefer to follow DefaultFlagsProvider#VERBOSE_EXCEPTION_SAMPLER_SPEC as the sensible default value.
There was a problem hiding this comment.
You might want to refer to
There was a problem hiding this comment.
I agree, Thanks.🙏
| * 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 { |
There was a problem hiding this comment.
How about?
Suggested change
| public final class TraceAbleRequestContextStorage implements RequestContextStorage { | |
| public final class LeakTracingRequestContextStorage implements RequestContextStorage { |
There was a problem hiding this comment.
That's a good name.
|
Do I need to do benchmarking for this pull request? |
+1 It would be good information to set the sensible default value of the sample rate. |
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 ```
|
Here is the benchmark result. Command ./gradlew --no-daemon :benchmarks:jmh:clean :benchmarks:jmh:jmh -Pjmh.includes=com.linecorp.armeria.common.LeakTracingRequestContextStorageBenchmarkMachine
Observation
From the data above, should I do something to fix/optimize it? By the way, I'm really new to benchmarking, so might fall into pitfalls. If something is incorrect with the benchmark test, please let me know. Thank you. 🙏 |
ikhoon
reviewed
May 2, 2022
Comment on lines
90
to
91
| delegate.pop(current, toRestore); | ||
| pendingRequestCtx.remove(); |
There was a problem hiding this comment.
Suggested change
| delegate.pop(current, toRestore); | |
| pendingRequestCtx.remove(); | |
| try { | |
| delegate.pop(current, toRestore); | |
| } finally { | |
| pendingRequestCtx.remove(); | |
| } |
Comment on lines
53
to
56
| Sampler<Class<? extends Throwable>> sampler) { | ||
| this.delegate = delegate; | ||
| pendingRequestCtx = new FastThreadLocal<>(); | ||
| this.sampler = sampler; |
There was a problem hiding this comment.
I don't see we need to limit the type to Class<Throwable>.
Suggested change
| Sampler<Class<? extends Throwable>> sampler) { | |
| this.delegate = delegate; | |
| pendingRequestCtx = new FastThreadLocal<>(); | |
| this.sampler = sampler; | |
| Sampler<?> sampler) { | |
| this.delegate = delegate; | |
| pendingRequestCtx = new FastThreadLocal<>(); | |
| this.sampler = (Sampler<Object>) sampler; |
core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java
Outdated
Show resolved
Hide resolved
ikhoon
reviewed
May 13, 2022
core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java
Outdated
Show resolved
Hide resolved
...race-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java
Outdated
Show resolved
Hide resolved
...race-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java
Outdated
Show resolved
Hide resolved
...race-context-leak/src/test/java/com/linecorp/armeria/common/TraceRequestContextLeakTest.java
Outdated
Show resolved
Hide resolved
ikhoon
approved these changes
May 16, 2022
ikhoon
reviewed
May 23, 2022
core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java
Outdated
Show resolved
Hide resolved
minwoox
reviewed
Aug 16, 2022
| prevContext.root() == toPush.root()) { | ||
| // The delegate has the ClientRequestContext whose root() is the same as toPush.root() | ||
| } else { | ||
| throw newIllegalContextPushingException(prevContext, toPush, pendingRequestCtx.get()); |
There was a problem hiding this comment.
Is this?
Suggested change
| throw newIllegalContextPushingException(prevContext, toPush, pendingRequestCtx.get()); | |
| throw newIllegalContextPushingException(toPush, prevContext, pendingRequestCtx.get()); |
core/src/main/java/com/linecorp/armeria/common/LeakTracingRequestContextStorage.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/internal/common/LeakTracingRequestContextStorage.java
Show resolved
Hide resolved
| // Reentrance | ||
| return noopSafeCloseable(); | ||
| } | ||
|
|
||
| final ServiceRequestContext root = root(); | ||
| if (oldCtx.root() == root) { |
There was a problem hiding this comment.
Shouldn't we also use unwrapAll?
There was a problem hiding this comment.
I think we should.
|
|
||
| private TraceableClientRequestContext(ClientRequestContext delegate) { | ||
| super(delegate); | ||
| stackTrace = new PendingRequestContextStackTrace(); |
There was a problem hiding this comment.
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
toStringis 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();
}There was a problem hiding this comment.
JFYI, StackTraceWriter uses 512 for the initial buffer.
armeria/core/src/main/java/com/linecorp/armeria/common/util/Exceptions.java
Lines 314 to 316 in 5b384fb
| // Reentrance | ||
| return noopSafeCloseable(); | ||
| } | ||
|
|
||
| if (oldCtx.root() == this) { |
…acingRequestContextStorage.java Co-authored-by: minux <[email protected]>
ikhoon
reviewed
Aug 16, 2022
Comment on lines
116
to
121
| 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(); |
ikhoon
reviewed
Aug 16, 2022
| import io.netty.util.AttributeKey; | ||
|
|
||
| /** | ||
| * This is copy from ClientRequestContextTest on core module to test behaviour consistency. |
There was a problem hiding this comment.
How about copying the files using Gradle's copy task?
armeria/spring/boot2-actuator-autoconfigure/build.gradle
Lines 18 to 21 in 0d3fa08
There was a problem hiding this comment.
Nice suggestion. Thanks!
| executor.execute(() -> { | ||
| final ServiceRequestContext ctx = newCtx("/1"); | ||
| try (SafeCloseable ignore = ctx.push()) { | ||
| //Ignore |
There was a problem hiding this comment.
Global comment: Could you add a white space after //?
Suggested change
| //Ignore | |
| // Ignore |
...ext-leak/src/test/java/com/linecorp/armeria/internal/common/TraceRequestContextLeakTest.java
Show resolved
Hide resolved
Comment on lines
110
to
111
| isThrown.set(true); | ||
| exception.set(ex); |
There was a problem hiding this comment.
For correct happen-before relationship.
Suggested change
| isThrown.set(true); | |
| exception.set(ex); | |
| exception.set(ex); | |
| isThrown.set(true); |
There was a problem hiding this comment.
Oh! Thanks for pointing out that.🙏
Comment on lines
141
to
142
| //Leak happened on the first eventLoop shouldn't affect 2nd eventLoop when trying to push | ||
| final ServiceRequestContext anotherCtx = newCtx("/2"); |
There was a problem hiding this comment.
Should we wait until latch.countDown() is invoked?
minwoox
reviewed
Aug 30, 2022
| @@ -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()) { | |||
There was a problem hiding this comment.
Let's also check if contextInThreadLocal is null or not just in case pop is called without push.
| RequestContext unwrap) { | ||
| final StringBuilder builder = new StringBuilder(512); | ||
| builder.append(unwrap).append(System.lineSeparator()) | ||
| .append("At thread [").append(threadName) |
There was a problem hiding this comment.
I don't think we need threadName because it's the same thread that raises newIllegalContextPoppingException . (If the thread is changed, the exception is not raised. 😉 )
There was a problem hiding this comment.
Ok, I got it. Thanks
Comment on lines
205
to
208
| 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); | ||
| } |
There was a problem hiding this comment.
This will create additional instances every time the method is called. How about
Suggested change
| 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; | |
| } |
There was a problem hiding this comment.
I can hardly say that it is convenient/easy to compare wrapped objects with this utility method.
How about adding a helper method to Unwrappable like
interface Unwrappable {
// The method name was inspired by String.equalsIgnoreCase().
// Better naming is welcome. 😆
@UnstableApi
boolean equalsIgnoreWrapper(@Nullable Unwrappable other) {
}
}so that we can easily compare the ctxs without complex if else conditions that might distract other important code.
it/trace-context-leak/build.gradle
Outdated
| include '**/ClientRequestContextTest.java' | ||
| } | ||
|
|
||
| tasks.compileJava.dependsOn(generateSources) |
There was a problem hiding this comment.
I miss it again 😅. Thanks.
minwoox
approved these changes
Sep 5, 2022
jrhee17
approved these changes
Sep 5, 2022
| return delegate; | ||
| } | ||
|
|
||
| private static RequestContextWrapper<?> warpRequestContext(RequestContext ctx) { |
There was a problem hiding this comment.
Suggested change
| private static RequestContextWrapper<?> warpRequestContext(RequestContext ctx) { | |
| private static RequestContextWrapper<?> wrapRequestContext(RequestContext ctx) { |
There was a problem hiding this comment.
Oh! misspelled. Thanks 🙏
| * A {@link RequestContextStorage} which keeps track of {@link RequestContext}s, reporting pushed thread | ||
| * information if a {@link RequestContext} is leaked. | ||
| */ | ||
| @UnstableApi |
There was a problem hiding this comment.
Since this class isn't part of the public api
Suggested change
| @UnstableApi |
|
Thanks to all reviewers for these meaningful suggestions! 🙇🙇🙇 |
|
Thanks for not giving up despite all the misdirection. 🤣 |
heowc
pushed a commit
to heowc/armeria
that referenced
this pull request
Sep 24, 2022
Motivation: Armeria has some core logic which assumes that the concrete default implementations of `[Client|Service]RequestContext` are `Default[Client|Service]RequestContext`. e.g. If the `ClientRequestContext` isn't an instance of `DefaultClientRequestContext`, then the response timeout may not be respected. https://github.com/line/armeria/blob/5edaef039abfd047f152a63cde38b46094c8353f/core/src/main/java/com/linecorp/armeria/client/HttpResponseDecoder.java#L401-L407 This usage pattern mostly stems from the fact that `[Client|Service]RequestContext` are public APIs that users are exposed to a lot (especially in decorators). However, Armeria developers are skeptical of adding public methods that are either 1) very specific and used only in core logic 2) have the potential to break behavior if used incorrectly. Up to now, we have added such methods to the default implementation and checked with `instanceof` when invoking such methods. With the introduction of line#4232, we are now planning on introducing and extensively utilizing wrapper classes for `[Client|Service]RequestContext`. However, using wrapper classes means previous assumptions of `instanceof Default[Client|Service]RequestContext` will be invalid. In this pull request, I propose that: 1) `RequestContext` now extends `Unwrappable`, allowing users to peel `RequestContext`s to the intended type. 2) We expose another interface `[Client]RequestContextExtension`, which contains "extension" methods for `ClientRequestContext`, `RequestContext`. In this interface, we can add methods that Armeria uses internally without worrying about public API exposure. 3) `Default[Client|Service]RequestContext` doesn't need to be exposed as a public API. We may simply expose the interface to users and hide the detailed implementation like we do for the `Http[Response|Request]` series. - Note: This move was handled in this PR mainly due to `CancellationScheduler`. While extracting `ClientRequestContextExtension`, `CancellationScheduler` which is an internal class was exposed as the return value of `#responseCancellationScheduler`. I had the choice of exposing `CancellationScheduler`, or hiding `DefaultClientRequestContext`. I chose the latter and also moved `DefaultServiceRequestContext` for consistency. Modifications: - `RequestContext` now extends `Unwrappable`. `RequestContextWrapper` now extends `AbstractUnwrappable` - Convert usages of `instanceof DefaultClientRequestContext` to unwrap instead - e.g. `ctx instanceof DefaultClientRequestContext` -> `ctx.as(ClientRequestContextExtension.class)` - `RequestContextExtension`, `ClientRequestContextExtension` has been added to an internal package - `DefaultClientRequestContext`, `DefaultServiceRequestContext`, `NonWrappingRequestContext` have been moved to an internal package - `pathWithQuery` is moved to `ClientUtil` and made public - `ClientThreadLocalState` is moved to an internal package and made public - Class hierarchy has been modified for `RequestContext` - Now there is an extra layer between `ClientRequestContext` and `DefaultClientRequestContext`: - `ClientRequestContext` -> `ClientRequestContextExtension` -> `DefaultClientRequestContext` - Now there is an extra layer between `RequestContext` and `NonWrappingRequestContext` - `RequestContext` -> `RequestContextExtension` -> `NonWrappingRequestContext` Before: <img width="300" alt="Screen Shot 2022-06-24 at 12 00 49 PM" src="https://user-images.githubusercontent.com/8510579/175453672-15dbdf09-4c2e-4afa-a186-a2147e97afa2.png"> After: <img width="300" alt="Screen Shot 2022-06-24 at 12 01 15 PM" src="https://user-images.githubusercontent.com/8510579/175453689-3c00edde-7209-4d1c-9480-e776c58537e0.png"> Result: - Developers may add more utility functions to `RequestContext` without worrying about public API exposure - Armeria tries to unwrap `RequestContext` instead of simply checking the type.
heowc
pushed a commit
to heowc/armeria
that referenced
this pull request
Sep 24, 2022
…ine#4232) Motivation: - Context leaks are hard to find because an exception does not tell where/which context is pushed without poping. By using TraceAbleRequestContextStorage, it helps to report the source of context leaks. - Details as mentioned in line#4100 By the way, Thanks to @anuraaga for giving a reference to read on [opentelemetry](https://github.com/open-telemetry/opentelemetry-java). Modifications: - Add `TraceAbleRequestContextStorage` that stores `RequestContext` stack trace and reports to the user where it happens. - Add `requestContextLeakDetectionSampler` flag that users can use for enable leak detection. Users can enable it by either system property or SPI flag provider. Result: - Closes line#4100 - `TraceAbleRequestContextStorage` is added, so users can use it to report where context leaks happen. How to enable: 1) By system property `-Dcom.linecorp.armeria.requestContextLeakDetectionSampler=<sampler-spec>` 2) By providing FlagsProvider SPI ```java public final class EnableLeakDetectionFlagsProvider implements FlagsProvider { @OverRide public Sampler<? super RequestContext> requestContextLeakDetectionSampler() { return Sampler.always(); } ... } ``` 3) By providing RequestContextStorageProvider SPI (not recommend since RequestContextStorageProvider SPI'll be remove as mentioned in #line#4211 ) ```java public final class CustomRequestContextStorageProvider implements RequestContextStorageProvider { @OverRide public RequestContextStorage newStorage() { return new TraceAbleRequestContextStorage(delegate); } } ``` Use case: Users problematic code ```java executor.execute(() -> { SafeCloseable leaked = fooCtx.push(); //This causes Request context leaks! ... }); executor.execute(() -> { try (SafeCloseable ignored = barCtx.push()) { //Exception happen here ... } }); ``` The above code will produce an error as below. Therefore, users can check the stack trace that which line causes context leaks. ``` java.lang.IllegalStateException: Trying to call object wrapped with context [%New RequestContext%], but context is currently set to TraceableServiceRequestContext[%Previous RequestContext%] com.linecorp.armeria.internal.common.LeakTracingRequestContextStorage$PendingRequestContextStackTrace: At thread [armeria-testing-eventloop-nio-1-1] previous RequestContext is pushed at the following stacktrace at com.linecorp.armeria.internal.common.LeakTracingRequestContextStorage$TraceableServiceRequestContext.<init>(LeakTracingRequestContextStorage.java:111) at com.linecorp.armeria.internal.common.LeakTracingRequestContextStorage$TraceableServiceRequestContext.<init>(LeakTracingRequestContextStorage.java:105) at com.linecorp.armeria.internal.common.LeakTracingRequestContextStorage.warpRequestContext(LeakTracingRequestContextStorage.java:82) at com.linecorp.armeria.internal.common.LeakTracingRequestContextStorage.push(LeakTracingRequestContextStorage.java:62) at com.linecorp.armeria.internal.common.RequestContextUtil.getAndSet(RequestContextUtil.java:149) at com.linecorp.armeria.server.ServiceRequestContext.push(ServiceRequestContext.java:221) at com.linecorp.armeria.internal.common.TraceRequestContextLeakTest.lambda$singleThreadContextLeak$2(TraceRequestContextLeakTest.java:101) <- This is the line where leaked RequestContext is push ... . This means the callback was called from unexpected thread or forgetting to close previous context. at com.linecorp.armeria.internal.common.RequestContextUtil.newIllegalContextPushingException(RequestContextUtil.java:100) at com.linecorp.armeria.server.ServiceRequestContext.push(ServiceRequestContext.java:237) at com.linecorp.armeria.internal.common.TraceRequestContextLeakTest.lambda$singleThreadContextLeak$3(TraceRequestContextLeakTest.java:107) ... ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
By the way, Thanks to @anuraaga for giving a reference to read on opentelemetry.
Modifications:
TraceAbleRequestContextStoragethat storesRequestContextstack trace and reports to the user where it happens.requestContextLeakDetectionSamplerflag that users can use for enable leak detection. Users can enable it by either system property or SPI flag provider.Result:
TraceAbleRequestContextStorageis added, so users can use it to report where context leaks happen.How to enable:
By system property
-Dcom.linecorp.armeria.requestContextLeakDetectionSampler=<sampler-spec>By providing FlagsProvider SPI
RequestContextStorageProviderlookup #4211 )Use case:
Users problematic code
The above code will produce an error as below. Therefore, users can check the stack trace that which line causes context leaks.