Skip to content

Commit

Permalink
Make zipkin's current context can be nested
Browse files Browse the repository at this point in the history
Apply this change will fix TraceContext leak in thread problem.
There is a case that we keep armeria's request context aware CompletableFuture
for fetching data, and let multiple request register callback on that future.
So there is a problem that future uses first request's eventloop and RequestContext
to call waiting handlers, but each handler may wrap with it's RequestContext, so
there is nested RequestContext on same thread(onEnter->onEnter->onExit->onExit).

Another solution is application developer should always jump back to current request's
context aware eventloop. e.g.

```

cachedFuture.acceptAsync((r, e)->{
  ...
}, RequestContext.current.contextAwareEventloop);

```

I'm not sure if armeria side need to take care of this or not...
  • Loading branch information
kojilin committed Jun 17, 2018
1 parent 0a24637 commit e27a778
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import com.linecorp.armeria.common.logging.RequestLogAvailability;
import com.linecorp.armeria.internal.tracing.AsciiStringKeyFactory;
import com.linecorp.armeria.internal.tracing.SpanContextUtil;
import com.linecorp.armeria.internal.tracing.SpanInScopeWrapper;

import brave.Span;
import brave.Span.Kind;
Expand All @@ -52,7 +53,7 @@
*/
public class HttpTracingClient extends SimpleDecoratingClient<HttpRequest, HttpResponse> {

private static final FastThreadLocal<SpanInScope> SPAN_IN_THREAD = new FastThreadLocal<>();
private static final FastThreadLocal<SpanInScopeWrapper> SPAN_IN_THREAD = new FastThreadLocal<>();

/**
* Creates a new tracing {@link Client} decorator using the specified {@link Tracing} instance.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,23 @@ public final class SpanContextUtil {
/**
* Sets up the {@link RequestContext} to push and pop the {@link Span} whenever it is entered/exited.
*/
public static void setupContext(FastThreadLocal<SpanInScope> threadLocalSpan, RequestContext ctx, Span span,
Tracer tracer) {
ctx.onEnter(unused -> threadLocalSpan.set(tracer.withSpanInScope(span)));
public static void setupContext(FastThreadLocal<SpanInScopeWrapper> threadLocalSpan, RequestContext ctx,
Span span, Tracer tracer) {
ctx.onEnter(unused -> {
final SpanInScopeWrapper current = threadLocalSpan.get();
final SpanInScope newScope = tracer.withSpanInScope(span);
threadLocalSpan.set(new SpanInScopeWrapper(newScope, current));
});
ctx.onExit(unused -> {
final SpanInScope spanInScope = threadLocalSpan.get();
if (spanInScope != null) {
spanInScope.close();
final SpanInScopeWrapper spanInScope = threadLocalSpan.get();
if (spanInScope == null) {
return;
}
spanInScope.close();
final SpanInScopeWrapper previousScope = spanInScope.getPrevious();
if (previousScope != null) {
threadLocalSpan.set(previousScope);
} else {
threadLocalSpan.remove();
}
});
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package com.linecorp.armeria.internal.tracing;

import javax.annotation.Nullable;

import brave.Tracer.SpanInScope;

/**
* SpanInScope Wrapper for keeping previous span scope.
*/
public final class SpanInScopeWrapper implements AutoCloseable {

private final SpanInScope spanInScope;

@Nullable
private final SpanInScopeWrapper previous;

public SpanInScopeWrapper(SpanInScope current, @Nullable SpanInScopeWrapper previous) {
this.spanInScope = current;
this.previous = previous;
}

@Nullable
public SpanInScopeWrapper getPrevious() {
return previous;
}

@Override
public void close() {
spanInScope.close();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.linecorp.armeria.common.logging.RequestLogAvailability;
import com.linecorp.armeria.internal.tracing.AsciiStringKeyFactory;
import com.linecorp.armeria.internal.tracing.SpanContextUtil;
import com.linecorp.armeria.internal.tracing.SpanInScopeWrapper;
import com.linecorp.armeria.server.Service;
import com.linecorp.armeria.server.ServiceRequestContext;
import com.linecorp.armeria.server.SimpleDecoratingService;
Expand All @@ -46,7 +47,7 @@
*/
public class HttpTracingService extends SimpleDecoratingService<HttpRequest, HttpResponse> {

private static final FastThreadLocal<SpanInScope> SPAN_IN_THREAD = new FastThreadLocal<>();
private static final FastThreadLocal<SpanInScopeWrapper> SPAN_IN_THREAD = new FastThreadLocal<>();

/**
* Creates a new tracing {@link Service} decorator using the specified {@link Tracing} instance.
Expand Down

0 comments on commit e27a778

Please sign in to comment.