Initial context API #562

louiscryan · 2015-06-24T21:04:59Z

This is an initial skeleton for the proposed Context API, lets use this PR to iterate on what we want.

Here are some general goals....

Provide for cascading closure (similar to cancellation in Go's Context)
Simple & small enough that its OK to digest on Android
Provides some simple type-safety for values but strictly limits keys to strings
Interaction with ThreadLocal tries not to be 'magical', anything attached must be explicitly detached. GRPC guarantees that it's code will be safe in this regard.
Easy to adapt to your IOC container of choice

Fixes #262

ejona86 · 2015-06-24T21:16:39Z

core/src/main/java/io/grpc/Context.java

+   * context which is already closed.
+   *
+   * @throws java.lang.IllegalStateException if this context is not current.
+   * @return the current context.


"the current context" means "this" object?

'current' means the context currently available from thread local (or root if there is nothing on the thread local)

I think you need to specify whether that is before or after the detach. If it is before, then that is 'this' object.

When would you expect to use the returned Context?

It made some of the tests more succinct. E.g.

assertSame(next, root.detach());

but I could take or leave it.

ejona86 · 2015-06-25T19:59:02Z

@louiscryan, I think it would be useful to have some very simple examples of how an application would use the Context.

zhangkun83 · 2015-06-25T21:46:49Z

core/src/main/java/io/grpc/Context.java

+   *
+   * @param <T>
+   */
+  public static class Key<T> {


If we are having get() on this class, maybe rename it to "Field", and "name" arguments to "key"?

+1 key does seem strange here.

I find Field more strange than Key. I have little problem with Key, although I have said that the current get() means that if you aren't fully aware of its type you don't realize it is accessing the Context.

Except that it's a Key that has a key and a value which is rather odd. If Context is essentially a map of names to values, then perhaps we should just follow the Java map naming conventions rather than inventing our own terminology. Entry?

Entry is also strange to me.

So we can treat that as one more reason to not have get(). Or maybe we should name get() something else, like lookup().

I'm not sure that the name of the method is the problem. The naming would even be worse for a method name lookup that returns a Key.

??

get() returns T. lookup() would also return T. lookup() would lookup the value for the Key based on the ThreadLocal.

Ah I think I see my confusion ... Key has a defaultValue, not the value itself. I'm fine with the name Key in that case. Sorry for the noise :/

+1 to lookup() over get()

Renamed it to lookup() though it's worth pointing out that 'get' was not going to be part of the public interface.

louiscryan · 2015-06-27T00:44:27Z

PTAL

nmittler · 2015-06-29T17:58:11Z

core/src/main/java/io/grpc/Context.java

+   * Provide an AutoCloseable (via Closeable) for use with try-with-resource blocks that will
+   * close a context and detach it.
+   */
+  public class ContextCloseable implements Closeable {


nit: Maybe CloseableContext?

Oh nm ... this isn't a context.

louiscryan · 2015-07-10T01:20:02Z

OK. I've now implemented the API and improved tests where there gaps. PTAL

nmittler · 2015-07-10T16:27:13Z

core/src/main/java/io/grpc/Context.java

+ * notified before listeners on the children.
+ *
+ */
+public class Context {


Just a discussion point: do we want this exposed at the top-level of the API in io.grpc? It doesn't seem like something that all users would necessarily use and isn't really part of the API in general. Would it make more sense in a subpackage?

I think one issue is that the functionality here isn't really specific to GRPC, at some point it could easily move into it's own thing. Given that and the fact that the package would only have one or two classes in it I'm not sure it makes much of a difference either way.

Yup, fair enough. I said that realizing that there would be nothing else in a context package or module, which would seem rather odd. We can shelve this for now and defer to some later conversation on how to clearly identify what classes and interface compose our "public" API.

ejona86 · 2015-07-10T20:37:57Z

core/src/main/java/io/grpc/Context.java

+        @Override
+        public ScheduledExecutorService create() {
+          return Executors.newScheduledThreadPool(1, new ThreadFactoryBuilder()
+              .setNameFormat(name + "-%d").build());


setDaemon(true)

ejona86 · 2015-07-13T22:09:44Z

core/src/main/java/io/grpc/Context.java

+  }
+
+  /**
+   * Return the cause of the cancellation or {@code null} if context was cancelled without a cause.


Define behavior if context not closed.

a-veitch · 2015-07-15T20:21:55Z

This is very different conceptually from the current way context-ish things are done in the C++/C side of gRPC. On that side, I see different context classes for client and server, and several first class components (deadline, auth context, and census context (which will include tracing context, tags etc.); the latter two are at least partially opaque), plus arbitrary key/value metadata pairs. Contexts are not immutable. Is there a desire to have the API's be similar in API/usage, or is this a non-goal?

louiscryan · 2015-07-15T21:05:54Z

I think the design tradeoffs for Java are different than those for C/C++
and what would be idiomatic in one would not necessarily be idiomatic for
the other. The goal is of course to provide the same concepts and the same
functionality across languages but to allow them to vary in idiom.

For Java we haven't seen a need to have a different class between client
and server, this seems like a simplification.
We have first-class support for deadline (see the withDeadline,
withCancellation methods)
We are able to achieve strongly-typed bindings for auth/tracing etc
without requiring explicit support for them in the Context class itself
(see the Key class)
We support mutabilty but the mechanism is different than in C++. This was
done for performance and footprint reasons and is based on the assumption
that reads are more common than mutations. We do not put an immutability
restriction on data stored in the context and folks are free to optimize
these for their use-cases as they see fit.

On Wed, Jul 15, 2015 at 1:22 PM, Alistair Veitch [email protected]
wrote:

This is very different conceptually from the current way context-ish
things are done in the C++/C side of gRPC. On that side, I see different
context classes for client and server, and several first class components
(deadline, auth context, and census context (which will include tracing
context, tags etc.); the latter two are at least partially opaque), plus
arbitrary key/value metadata pairs. Contexts are not immutable. Is there a
desire to have the API's be similar in API/usage, or is this a non-goal?

—
Reply to this email directly or view it on GitHub
#562 (comment).

louiscryan · 2015-07-16T20:22:54Z

Given that we need to make some forward progress on context binding within GRPC itself I'll be checking this in soon. The GRPC APIs are not frozen yet so there's plenty of scope for change and feedback so I encourage folks to get their hands dirty with it.

codefromthecrypt · 2015-07-16T23:40:41Z

nice work

…mount of state through the call stack and across thread boundaries. Strongly modeled after the Go context API https://blog.golang.org/context with support for - cancellation propagation & cancellation listeners - typed value binding - timeout/deadline The major difference with Go is that ThreadLocal is used for propagation instead of parameter passing as this is considered more idiomatic for Java.

louiscryan · 2015-07-21T17:25:04Z

Merged as f612116

ejona86 reviewed Jun 24, 2015
View reviewed changes

zhangkun83 reviewed Jun 25, 2015
View reviewed changes

nmittler reviewed Jun 29, 2015
View reviewed changes

nmittler reviewed Jul 10, 2015
View reviewed changes

louiscryan force-pushed the ContextAPI branch from e508cea to 6640976 Compare July 10, 2015 19:33

ejona86 reviewed Jul 10, 2015
View reviewed changes

ejona86 reviewed Jul 13, 2015
View reviewed changes

This was referenced Jul 16, 2015

A standard for continuation-local variables? ReactiveX/RxJava#2885

Closed

Initial implementation of ContinuationLocal DistributedTracing/continuation-local-storage-jvm#1

Open

louiscryan force-pushed the ContextAPI branch from a0b5236 to 9988077 Compare July 16, 2015 20:18

louiscryan force-pushed the ContextAPI branch from 9988077 to 4efa87b Compare July 21, 2015 17:20

louiscryan closed this Jul 21, 2015

lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial context API #562

Initial context API #562

louiscryan commented Jun 24, 2015

ejona86 Jun 24, 2015

louiscryan Jun 24, 2015

ejona86 Jun 24, 2015

louiscryan Jun 24, 2015

ejona86 commented Jun 25, 2015

zhangkun83 Jun 25, 2015

nmittler Jun 26, 2015

ejona86 Jun 26, 2015

nmittler Jun 26, 2015

ejona86 Jun 26, 2015

nmittler Jun 26, 2015

ejona86 Jun 26, 2015

nmittler Jun 26, 2015

jhump Jun 29, 2015

louiscryan Jul 9, 2015

louiscryan commented Jun 27, 2015

nmittler Jun 29, 2015

nmittler Jun 29, 2015

louiscryan commented Jul 10, 2015

nmittler Jul 10, 2015

louiscryan Jul 10, 2015

nmittler Jul 10, 2015

ejona86 Jul 10, 2015

louiscryan Jul 10, 2015

ejona86 Jul 13, 2015

louiscryan Jul 13, 2015

a-veitch commented Jul 15, 2015

louiscryan commented Jul 15, 2015

louiscryan commented Jul 16, 2015

codefromthecrypt commented Jul 16, 2015 via email

louiscryan commented Jul 21, 2015

Initial context API #562

Initial context API #562

Conversation

louiscryan commented Jun 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 commented Jun 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louiscryan commented Jun 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louiscryan commented Jul 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-veitch commented Jul 15, 2015

louiscryan commented Jul 15, 2015

louiscryan commented Jul 16, 2015

codefromthecrypt commented Jul 16, 2015 via email

louiscryan commented Jul 21, 2015