Skip to content
This repository was archived by the owner on May 6, 2020. It is now read-only.

Extract ThreadContext from Context and add UntypedContext#1

Open
trask wants to merge 3 commits into
propagationio:masterfrom
trask:support-more-use-cases
Open

Extract ThreadContext from Context and add UntypedContext#1
trask wants to merge 3 commits into
propagationio:masterfrom
trask:support-more-use-cases

Conversation

@trask
Copy link
Copy Markdown
Member

@trask trask commented May 5, 2020

I recommend to start by reviewing the updated README in this PR, which tries to explain
motivation, copied here:


Propagation.io

A standalone context propagation library for Java that is focused on improving interoperability across existing context propagation APIs like gRPC Context and Reactor.

Design questions

Should the context have a thread-based "current" concept?

Having a thread-based "current" context makes sense in some environments, but doesn't make sense in others.

Reflecting that, some existing libraries like gRPC Context have this concept, while some existing libraries like Reactor do not.

So, in order to improve interoperability across existing context propagation APIs, the current proposal provides both options on top of the same immutable data structure in order to make transitioning between them efficient (see Context and ThreadContext).

Should the context have typed keys?

Having typed keys seems to be generally preferable (we are interested to learn more about use cases where they are not!).

However, some existing libraries like gRPC Context use typed keys, and some existing libraries like Reactor do not.

So, in order to improve interoperability across existing context propagation APIs, the current proposal provides both options on top of the same immutable data structure in order to make transitioning between them efficient (see Context and UntypedContext).

[Note: there's no UntypedThreadContext yet, we need to investigate more libraries and hear from more folks to know if this is needed.]

Should this project support Java 7?

Supporting Java 7 limits some design choices, but is a requirement for both gRPC and OpenTelemetry.

Should context be an interface or an abstract class?

Since we are limited to Java 7, Context, ThreadContext, and UntypedContext have been implemented as abstract classes instead of interfaces so that their related static methods can be collocated in the same place, making the API surface feel smaller and improving discoverability.

Alternatively, we could make them interfaces and move the static methods to corresponding util classes (e.g. Contexts, ThreadContexts, and UntypedContexts), which would give implementations more flexibility.


(End of README)

And then here are some notes that hopefully help while reviewing the code:

Context

  • By moving generation to ThreadContext, was able to implement Context directly using Node which improves memory and helps optimize interop between the different context implementations. My thought was that generation issues are probably more of an issue when binding the context to the thread, compared to passing the context around directly.
  • Intellij complained when I put constant EMPTY in this class ("Referencing subclass from superclass initializer might lead to class loading deadlock") so I made this a static method empty() instead.

ThreadContext

  • An alternative name could be CurrentContext, but I liked ThreadContext because it's explicit about being tied to the thread, and "current" is a little more vague, (e.g. current subscriber context in Reactor?)
  • Same issue as above with constant EMPTY/ROOT in this class. Named the method empty() instead of root() since ThreadContext extends Context, and I wanted the method to hide Context.empty(). Also, it's not quite a "root" in the same sense that it used to be before the ThreadContext split.
  • Extends Context, and the withValues(..) methods are overridden to narrow the return type.
    • This works out nicely for APIs that accept and return Context. The APIs can be written against Context, and if the API user is using ThreadContext, the user can pass ThreadContext directly to the API. And if the API calls withValue on the context that was passed in and returns a new context, the returned context will still be a ThreadContext, so that when the user calls ThreadContext.wrap(context) to convert the returned Context back into a ThreadContext it will essentially be a no-op.
  • Renamed wrap(Runnable) and wrap(Callable) to wrapRunnable(Runnable) and wrapCallable(Callable) so that they wouldn't look quite so similar to the static wrap(Context) method, but maybe still not ideal.

ThreadBinding

  • Split out from DefaultThreadContext just to keep classes smaller and more narrowly scoped (no strong opinion here).

ThreadContextStorage

  • Extracted ThreadContext.Storage out into top-level class so that it doesn't take up space in ThreadContext (no strong opinion here either).

Context.Key

  • Typed contexts do not have public getter, value must be accessed through Key, this is not consistent with untyped context, which has getter, but I think this difference could be good, as it emphasizes the fundamental difference between typed and untyped contexts.
  • Class changed to non-final so that it can be subclassed by ThreadContext.Key.

ThreadContext.Key

  • Extends Context.Key to add convenience no-arg getter that uses ThreadContext.current().

These are the methods for converting between the different context types. I think the naming and location (instance vs static) could likely be improved.

  • Context → ThreadContext
    • ThreadContext.wrap(context)
    • This is not an instance method on Context because that felt like leaking thread binding concept into the Context class.
    • Not totally sold on the "wrap/unwrap" terminology.
    • Could be renamed ThreadContext.of(Context) but not sure what the opposite "unwrap" method would be named in this case.
  • ThreadContext → Context
    • threadContext.unwrap()
    • Like mentioned above, not totally sold on the "wrap/unwrap" terminology.
  • Context → UntypedContext
    • context.toUntyped()
    • This isn't a static wrap method on UntypedContext (similar to ThreadContext above), because it doesn't work for anything that implements Context, it's only translates between very specific implementations of Context and UntypedContext.
  • UntypedContext → Context
    • untypedContext.toTyped()

The existing gRPC Context tests have been adapted to test ThreadContext. There are no tests yet for Context or UntypedContext, this will need to be addressed.

Probably lots more stuff that I'm not thinking of or haven't noticed.

Feel free to comment on both big design issues and small implementation/doc issues. All types of feedback is welcome and needed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant