-
Notifications
You must be signed in to change notification settings - Fork 926
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
Changes from 30 commits
1d64827
c79b852
443e740
c431103
06c728e
0f5e5e5
bcf92df
82bb1a8
a7fd563
ea2e675
cb8dcf4
50d4127
818c166
5207a4c
d24e0a2
61e767d
449f051
4dabd01
8909ace
fdd8490
5562cfe
f124fc8
a71a725
c4a534f
3b00c2a
af31a6e
ab4de84
9f681d4
a72a9ca
d43b052
4f8a8f2
6cbcfab
89ddc46
6b4b9f9
083d560
4fb9575
7b3f3d1
9d929bf
90e6e78
a0540c8
9156898
50384a2
0fe21fd
caba3d6
1302481
9e8e10e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's also check if |
||
throw newIllegalContextPoppingException(current, contextInThreadLocal); | ||
} | ||
context.set(map, toRestore); | ||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -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 | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this class isn't part of the public api
Suggested change
|
||||||||
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) { | ||||||||
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) { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about using
If we using 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. JFYI, armeria/core/src/main/java/com/linecorp/armeria/common/util/Exceptions.java Lines 314 to 316 in 5b384fb
|
||||||||
} | ||||||||
|
||||||||
@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(); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"); | ||||||||
} | ||||||||
} | ||||||||
} |
There was a problem hiding this comment.
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
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should.