concurrent-api: cleanup AsyncContext operations #3181
Conversation
Motivation: Perhaps the main goal of the AsyncContext is to allow a way to bundle up thread-local (ish) state and restore it when an async operation occurs. However, restore operation is essentially inlined in all the context operations which makes the mechanism more difficult to understand than it needs to be. Modifications: Add a few new methods to the AsyncContextProvider - saveContext() the goal of this method is to package up context for propagation to the next continuation. This stands in contrast to the existing context() method which is used for reading the async ContextMap. - attachContext(ContextMap) this method is used to restore the local state for temporary use. It returns a new Scope type, which then reverts this local environment to what it was before the restore. Result: Cleaner async context operations.
| * wrapped to bundle up additional contexts such as the OpenTelemetry or grpc contexts. | ||
| * @return the saved context state that may be restored later. | ||
| */ | ||
| ContextMap saveContext(); |
There was a problem hiding this comment.
In an ideal world we would make the fact that this is a ContextMap abstract and have an interface that looks something like this
interface SavedContext {
Scope restoreContext();
}
However, our own uses are relatively complex and furthermore people are already writing agent code that make this change difficult. See the DataDog java-agent implementation here which depends on the saved context state being a ContextMap.
There was a problem hiding this comment.
See bryce-anderson@5806c78 for a commit that unwinds current usage of the ContextMap directly. Note that there would still be a bit more to do.
There was a problem hiding this comment.
I'm fine with the current name, just want to propose other options for consideration: wdyt about naming it grabContext() or captureContext()? Will those sound more like we are taking a snapshot in preparation for a jump?
servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/AsyncContext.java
Outdated
Show resolved
Hide resolved
servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/Completable.java
Outdated
Show resolved
Hide resolved
| * wrapped to bundle up additional contexts such as the OpenTelemetry or grpc contexts. | ||
| * @return the saved context state that may be restored later. | ||
| */ | ||
| ContextMap saveContext(); |
There was a problem hiding this comment.
I'm fine with the current name, just want to propose other options for consideration: wdyt about naming it grabContext() or captureContext()? Will those sound more like we are taking a snapshot in preparation for a jump?
...lk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/ContextAwareExecutorUtils.java
Outdated
Show resolved
Hide resolved
...-concurrent-api/src/main/java/io/servicetalk/concurrent/api/ContextPreservingBiConsumer.java
Outdated
Show resolved
Hide resolved
...rent-api/src/main/java/io/servicetalk/concurrent/api/ContextPreservingCompletableFuture.java
Outdated
Show resolved
Hide resolved
|
|
||
| import javax.annotation.Nullable; | ||
|
|
||
| import static io.servicetalk.concurrent.api.AsyncContextMapThreadLocal.CONTEXT_THREAD_LOCAL; | ||
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| class ContextPreservingSingleSubscriber<T> implements Subscriber<T> { |
There was a problem hiding this comment.
Not necessary for now, but whenever you have time could you please verify that we indeed call AsyncContextProvider.wrapSingleSubscriber(...) and similar wrappers for Completable/Publisher in all operators whey it's necessary. Would be great to make sure we don't miss context propagation.
...-concurrent-api/src/main/java/io/servicetalk/concurrent/api/DefaultAsyncContextProvider.java
Outdated
Show resolved
Hide resolved
...-concurrent-api/src/main/java/io/servicetalk/concurrent/api/DefaultAsyncContextProvider.java
Outdated
Show resolved
Hide resolved
Motivation:
Perhaps the main goal of the AsyncContext is to allow a way to bundle up thread-local (ish) state and restore it when an async operation occurs. However, restore operation is essentially inlined in all the context operations which makes the mechanism more difficult to understand than it needs to be.
Modifications:
Add a few new methods to the AsyncContextProvider
Result:
Cleaner async context operations.