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 methods to ServiceInvocationContext for propagating context into callback invocations. #79

Merged
merged 1 commit into from
Jan 7, 2016

Conversation

anuraaga
Copy link
Collaborator

@anuraaga anuraaga commented Jan 5, 2016

Currently, when writing an asynchronous service that does multiple RPCs, the RPC callbacks will likely be called with no ServiceInvocationContext.current() set. This is because the initial invocation removes the context right away to allow processing another event from the event loop, here

https://github.com/line/armeria/blob/master/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java#L264

We need a mechanism for propagating the invocation context to when the RPC callbacks fire (and may trigger additional RPCs, callbacks, etc), otherwise context-dependent features (think zipkin) will not work.

This PR adds methods for propagating context into callback methods, including an executor that will do it automatically for any subitted tasks. In general, service code will want to to use this executor in calls like CompletableFuture.whenCompleteAsync. They probably already are using ServiceInvocationContext.eventLoop to make sure callbacks are called on the same thread, and this would replace that so they're on the same thread with the correct context.

public final Executor asyncCallbackExecutor() {
return command -> {
eventLoop().execute(() -> {
setCurrent(this);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me know if I should check the current context or something. As far as I would expect, eventLoop.execute() is guaranteed to execute the task at some time in the future after the current calling code has returned since it's all just one thread, meaning there's no way this could be set at this time.

@trustin
Copy link
Member

trustin commented Jan 5, 2016

Instead, you could get the current ServiceInvocationContext before creating a callback and access the context from the callback:

public class MyThriftServiceImpl implements MyThriftService.Iface {
    public void foo() {
        ServiceInvocationContext ctx = ServiceInvocationContext.current();
        ListenableFuture<Object> f = someAsyncClientLibrary.bar();
        f.addListener(() -> {
            ... do something with ctx ...
        }, someExecutor);
    }
}

Or.. am I missing something out?

@anuraaga
Copy link
Collaborator Author

anuraaga commented Jan 5, 2016

And call ServiceInvocationContext.setCurrent in service code? That sounds wrong.

Again I'm mostly concerned with armeria breaking because of the lack of context for stuff like tracing, and having the client code having to call setCurrent or whatever sounds like a dangerous way to make that work.

@trustin
Copy link
Member

trustin commented Jan 5, 2016

And call ServiceInvocationContext.setCurrent in service code? That sounds wrong.

No. Just passing the context object as a field of a callback?

@anuraaga
Copy link
Collaborator Author

anuraaga commented Jan 5, 2016

How would armeria client's use of current ServiceInvocationContext to stay on the thread work? And in the future opening a zipkin span in the context. Both of these happen in armeria and rely on setCurrent having been called. I think propagating inside an armeria executor makes the most sense for them to continue to work.

@anuraaga
Copy link
Collaborator Author

anuraaga commented Jan 5, 2016

Specifically this code currently won't work in callbacks

@anuraaga anuraaga changed the title Add a ServiceInvocationContext propagating executor. [WIP] Add a ServiceInvocationContext propagating executor. Jan 6, 2016
@anuraaga anuraaga changed the title [WIP] Add a ServiceInvocationContext propagating executor. [WIP] Add methods to ServiceInvocationContext for propagating context into callback invocations. Jan 6, 2016
"Trying to call context-aware callback with context " + propagatedContext +
", but context is currently set to " + currentContext + ". This means the " +
"callback was passed from one invocation to another which is not allowed. Make " +
"sure you are not saving callbacks into shared state.");
Copy link
Member

Choose a reason for hiding this comment

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

callback -> object created by makeContextAware() ? How about rephrasing this so it mentions makeContextAware() and contextAwareEventExecutor()?

@trustin trustin added this to the 0.7.0.Final milestone Jan 6, 2016
@anuraaga anuraaga changed the title [WIP] Add methods to ServiceInvocationContext for propagating context into callback invocations. Add methods to ServiceInvocationContext for propagating context into callback invocations. Jan 6, 2016
@@ -149,6 +154,116 @@ public final EventLoop eventLoop() {
}

/**
* Returns a {@link EventExecutor} that will make sure this invocation is set
Copy link
Member

Choose a reason for hiding this comment

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

a -> an

@anuraaga
Copy link
Collaborator Author

anuraaga commented Jan 7, 2016

Addressed comments

trustin added a commit that referenced this pull request Jan 7, 2016
Add methods to ServiceInvocationContext for propagating context into callback invocations.
@trustin trustin merged commit 07c120e into line:master Jan 7, 2016
@trustin
Copy link
Member

trustin commented Jan 7, 2016

@anuraaga Thanks! Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants