Armeria: `RequestContextAwareCompletableFuture` callback not invoked when context mismatches.

[trustin]

Let's say we have the following code:

ServiceRequestContext ctx1 = ServiceRequestContext.of(...);
ServiceRequestContext ctx2 = ServiceRequestContext.of(...);
try (SafeCloseable sc1 = ctx1.push()) {
    CompletableFuture f = ctx2.makeContextAware(new CompletableFuture());
    AtomicBoolean callbackCalled = new AtomicBoolean();
    f.whenComplete((res, cause) -> callbackCalled.set(true));

    f.complete(null);    

    // The callback will never be invoked and
    // the following assertion will fail.
    assert callbackCalled.get(); // <-- Failure!
}

It seems like everything is working as expected. The callback (BiConsumer) was not invoked because it was decorated by ctx2.makeContextAware(BiConsumer) and the callback is invoked only when ctx2.pushIfAbsent() succeeds (see), to protect our users from the accidental context conflict like the above example.

The problem is that a user does not have a way to know this happened. It is really confusing because the callback will never be invoked. It is neither possible to handle the IllegalStateException raised by pushIfAbsent():

f.whenComplete((res, cause) -> callbackCalled.set(true))
 .exceptionally(cause -> {
     // We never reach here, either, because this callback
     // will be decorated again and fail at `pushIfAbsent()` again.
     ...
 });

What would be the best way to let a user know about this problem?

[trustin]

How about logging at WARN level instead of raising an exception for the future callbacks? If a user wraps something explicitly via ctx.makeContextAware(), we could raise an IllegalStateException as before, though, i.e.

class AbstractRequestContext ... {
    // Stay same.
    BiConsumer makeContextAware(BiConsumer consumer) {
        return (t, u) -> {
            try (SafeCloseable ignored = pushIfAbsent()) {
                action.accept(t, u);
            }
        };
    }
}

class RequestContextAwareCompletableFuture ... {
    @Override
    public CompletableFuture<T> whenComplete(BiConsumer<? super T, ? super Throwable> action) {
        return ctx.makeContextAware(super.whenComplete(laxMakeContextAware(action)));
    }

    BiConsumer laxMakeContextAware(BiConsumer consumer) {
        return (t, u) -> {
            try (SafeCloseable ignored = push()) {
                action.accept(t, u);
            }
        };
    }

    SafeCloseable push() {
        final RequestContext oldCtx = RequestContext.currentOrNull();
        if (oldCtx != null && oldCtx != this.ctx) {
            // Warn. Do not raise an exception.
            if (!warned) {
                logger.warn(...);
                warned = true;
            }
        }
        return ctx.push();
    }
}

[trustin]

The problem described in #2282 is also related with this problem. I understand pushIfAbsent() provides some protection against unnecessarily nested contexts, but there are valid cases of nested contexts, such as:

• A service triggering a client request (described in this issue)
• A client triggering another client, i.e. RetryingClient (described in #2282)

Therefore, how about relaxing pushIfAbsent() to allow:

• pushing a client context when the current thread local context is a service context
• pushing a context when one of its parent contexts is the current thread local context.
  
  
  
  • We will have to add RequestContext.parent() to handle this case, where newDerivedContext() sets the parent context automatically. I'm not sure if this is the best idea though. Thoughts? @minwoox @ikhoon @anuraaga
  
  

[anuraaga]

I think these basically make sense but does it mean we need to make AttributeMap read from parents too? I don't know if it's ever good to completely overwrite the current context (it's what the ifAbsent check is protecting against), for example if a span is saved in the current context I think it still needs to be accessible after such a push.

[trustin]

That's a good question which leads us to #1083. Perhaps we should let a user specify whether an attribute value needs to be copied when a context is derived or nested? e.g.

static final AttributeKey<TraceContext> TRACE_CONTEXT_KEY =
        AttributeKey.valueOf(TraceContextUtil.class, "TRACE_CONTEXT",
                             /* inherited */ true);

We will have to fork/replace Netty AttributeKey to do this, though.

[minwoox]

pushing a context when one of its parent contexts is the current thread local context.

I think this should be:
pushing a context when it is one of the parent contexts of the current thread local context.

Yes, I think RequestContext.parent() is a good choice for that and also having the AttirbuteKey which a user can specify whether to copy or not looks good to me.

[anuraaga]

Do we have any examples where we wouldn't want to inherit attributes? I feel as if when a context is mounted, semantically it should always be mounted until popped - which is a little different than starting a thread which may or may not have a related lifecycle, so always inheriting seems reasonable. If it seems ok we wouldn't need to fork code keeping things simpler.

[trustin]

Yeah, we could default to 'always inherit' and then wait for a counter use case.

[trustin]

By the way, once we introduce RequestContext.parent() we might not need to copy the attributes. When not found, could just check the parent.

[anuraaga]

Ran into a somewhat related issue

https://github.com/openzipkin/zipkin/issues/2932#issuecomment-561021463

Just need to make sure that with what we do here, in the context of tracing, we don't end up with two client spans in a parent/child relationship. I don't think it's what we're planning here, but that discussion reminded me of this one :)

[minwoox]

we don't end up with two client spans in a parent/child relationship.

Had a chat with @trustin about this.
In the example described in #2282, the order of the contexts in thread-local will be:
ServiceRequestContext -> ClientRequestContext1 -> ClientRequestContext2
This will have the parent/child relationships between ClientReqeustContexts.

This will be not a problem if the ClientReqeustContext1 and ClientReqeustContext2 do not have any attribute and look up to find an attribute only in ServiceRequestContext. But it's a problem when ClientReqeustContext1 has the attribute that ClientReqeustContext2 is looking for.

So we thought the word parent is kinna vague to represent what it really is and
we needed to use the more specific term like ClientRequestContext.serviceContext() (or serviceRequestcontext?).
What do you think @anuraaga?

[anuraaga]

So if I understand correctly, a client context will either read from itself, or an ancestor service context in the context chain. That seems fine to me.

[minwoox]

Exactly, let me work on it. 😉

[minwoox]

How about logging at WARN level instead of raising an exception for the future callbacks?

That sounds good. Let me work on this after #2322 is merged.

[minwoox]

How about logging at WARN level instead of raising an exception for the future callbacks? If a user wraps something explicitly via ctx.makeContextAware(), we could raise an IllegalStateException as before, though, i.e.

Had a chat with @trustin and decided to leave warning logs multiple times to let the users know something is going wrong.
We will still throw an exception because otherwise some critical resources can be shared such as access token, etc.