context: change storage API to return a restoreable context #3292

zpencer · 2017-07-28T23:26:13Z

This API change allows storage implementations to put some state information into the return value, giving it the ability to act as a cookie. In environments where contexts can be replaced, the current original context can be stashed there and be restored when detach is called.

zhangkun83 · 2017-07-29T00:21:09Z

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

     */
-    public abstract void attach(Context toAttach);
+    public abstract Context attach(Context toAttach);


IIRC this is a binary-incompatible change (you may want to verify it). Although the storage API is experimental thus allows us to break, our practice has been that we will provide smooth transition as long as it doesn't take much effort.

If this is binary-incompatible, a trick I usually use is:

@Deprecated public void attach(Context toAttach) { throw new UnsupportedOperationException("Deprecated. Do not call."); } public Context doAttach(Context toAttach) { attach(toAttach); return Context.current(); }

After one release, delete attach() and make doAttach() abstract.

Performed a simple test and verified that changing a void return type to a non avoid return type is not binary compatible.

zhangkun83 · 2017-07-29T00:21:35Z

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

     */
-    public abstract void attach(Context toAttach);
+    public abstract Context attach(Context toAttach);


Context.attach() needs to use the returned Context.

codefromthecrypt

I think a test that uses functionality as described would help explain and also make sure it isnt broken later. I understand that the way storage is initialized, testing might be awkward.

codefromthecrypt · 2017-07-30T06:03:39Z

Curious what the driving use case is for this. For example, it is neat to stain a context, more interesting why you might need to?

A breaking change, but to understand this better, if the api was like this, it would make this more obvious?:

Object attach(Context toAttach)
void detatch(Object token, Context toRestore)

Ex the main point is you are passing back the token you got (otherwise things could be bad)

Aside, but the context storage api doesnt really say what happens when you have an incorrect pairing.

ejona86

Looks fine other than Kun's comments.

ejona86 · 2017-07-31T22:34:45Z

@adriancole wrote:

Curious what the driving use case is for this. For example, it is neat to stain a context, more interesting why you might need to?

We're using it to migrate a codebase from another context. The two ways we're using it: 1) to support replace() in the old API without losing the verification in detach() and 2) to save the old API's instance so we can return it later. (1) is rarely used and was basically legacy before we started, but is hard to migrate. It's questionable whether we need (2), but since we're doing the trick already it's one less risk.

A breaking change, but to understand this better, if the api was like this, it would make this more obvious?:

Object attach(Context toAttach)
void detach(Object token, Context toRestore)

Yep (although void detach(Object token)). Can't be done now though. That could have helped during earlier migration too. I'm not sure we would have done that, even if we knew then what we know now. But we would have considered it harder; we didn't really at the time (I think there may have been a 5 second consideration and a "meh").

Aside, but the context storage api doesnt really say what happens when you have an incorrect pairing.

If you have an incorrect pairing then the current() after the call isn't what is expected. detach() talks about the SEVERE log that happens when we detect such a breakage, but it is only after-the-fact; the log location isn't likely the source of the bug.

So the concern is more this case, that works without issue before the hack:

Context old = Context.current();
new.attach(); // throws away return :(
try {
  ...;
} finally {
  new.detach(old);
}

From an API perspective, it isn't clear if that is a bug, since attach() says:

The previously current context is returned.

While detach() says:

The provided replacement should be what was returned by the same attach() call.

We'll call it a bug internally; it seems to have no redeeming value and is easy to fix.

codefromthecrypt · 2017-08-01T00:33:18Z

Thank you for the detailed explanation, Eric. This helps me understand and I appreciate your taking the time.

…

On 1 Aug 2017 06:34, "Eric Anderson" ***@***.***> wrote: Curious what the driving use case is for this. For example, it is neat to stain a context, more interesting why you might need to? We're using it to migrate a codebase from another context. The two ways we're using it: 1) to support replace() in the old API without losing the verification in detach() and 2) to save the old API's instance so we can return it later. (1) is rarely used and was basically legacy before we started, but is hard to migrate. It's questionable whether we need (2), but since we're doing the trick already it's one less risk. A breaking change, but to understand this better, if the api was like this, it would make this more obvious?: Object attach(Context toAttach) void detach(Object token, Context toRestore) Yep (although void detach(Object token)). Can't be done now though. That could have helped during earlier migration too. I'm not sure we would have done that, even if we knew then what we know now. But we would have considered it; we didn't at the time. Aside, but the context storage api doesnt really say what happens when you have an incorrect pairing. If you have an incorrect pairing then the current() after the call isn't what is expected. detach() <https://grpc.io/grpc-java/javadoc/io/grpc/Context.html#detach-io.grpc.Context-> talks about the SEVERE log that happens when we detect such a breakage, but it is only after-the-fact; the log location isn't likely the source of the bug. So the concern is more this case, that works without issue before the hack: Context old = Context.current();new.attach(); // throws away return :(try { ...; } finally { new.detach(old); } From an API perspective, it isn't clear if that is a bug, since attach() says: The previously current context is returned. While detach() says: The provided replacement should be what was returned by the same attach() call. We'll call it a bug internally; it seems to have no redeeming value and is easy to fix. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3292 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD613a2iAV4WcOaP9xRShCud26GhtMoks5sTlaFgaJpZM4OnLG1> .

ejona86 · 2017-08-02T16:40:39Z

context/src/main/java/io/grpc/ThreadLocalContextStorage.java

+  private static final ThreadLocal<Context> localContext = new ThreadLocal<Context>() {
+    @Override
+    protected Context initialValue() {
+      return Context.ROOT;


This would likely bite any implementation, because current and attach are working differently. I see two options:

We remove the if (current == null) check in current() and require the storage to handle that case. We would still want a current == null check, but it would throw an exception instead of choosing ROOT.

Have Context.attach() check the return and use ROOT if it is null. This is functionally equivalent to what was being done before.

Let's go with option 2, since today implementations are allowed to return null and have Context handle it.

ejona86 · 2017-08-02T16:41:29Z

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

     */
-    public abstract void attach(Context toAttach);
+    public abstract Context doAttach(Context toAttach);


Adding a new method is not binary compatible. You can provide a default implementation that uses attach().

Note that because of the current definition of current(), you'd have to convert null to ROOT within doAttach().

codefromthecrypt · 2017-08-04T00:50:23Z

context/src/test/java/io/grpc/ContextTest.java

+   */
+  @Test
+  public void attachDetachNewThread() throws ExecutionException, InterruptedException {
+    Future<?> future = scheduler.submit(Context.current().wrap(new Runnable() {


seems this could at least check what's current? (from another test)

Context blue = Context.current().withValue(COLOR, "blue"); --snip-- public void run() { assertTrue(blue.isCurrent());

Also, if you are testing that it is per-thread structures, maybe start one future with a countdown latch. change the current context to RED, start another future which checks red is current, finally countdown the blue future's latch (which should still be blue)

Good suggestion on the current value, I will update the tests. As for per thread data structures, I was previously just referring to initializing the ThreadLocal to ROOT, which is not necessary anymore with ejona's suggestions.

- update tests - make Context handle NULL values

zhangkun83 · 2017-08-04T21:03:34Z

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

     */
-    public abstract void attach(Context toAttach);
+    public Context doAttach(Context toAttach) {


We should document that the return value can be null, and what Context.attach() will use in that case. I should have had the same comment on current() too.

codefromthecrypt · 2017-08-05T00:16:17Z

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

@@ -16,6 +16,7 @@

 package io.grpc;

+import com.sun.istack.internal.Nullable;


is this the nullable you want? :)

Ah I should be more careful about trusting my IDE. I will be removing this annotation as per Kun's comment about deps.

zhangkun83

Ah. Do not use @Nullable. We must have zero dependency.

codefromthecrypt · 2017-08-05T00:18:39Z

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

-     * Implements {@link io.grpc.Context#current}.  Returns the context of the current scope.
+     * Implements {@link io.grpc.Context#current}.
+     *
+     * <p>Caution: {@link Context} interprets a return value of {@code null} to mean the same


maybe make a test that via reflection changes Storage to something that returns null? then test that ROOT is substituted

zpencer · 2017-08-09T19:46:51Z

@ejona86 any more comments on this one?

zpencer · 2017-08-11T01:41:09Z

(jenkins) restest this please

zpencer · 2017-08-11T01:46:12Z

retest this please

context: change storage API to return a restoreable context

cd8d184

zpencer requested review from ejona86 and zhangkun83 July 28, 2017 23:26

fix indent level

3764aae

zhangkun83 requested changes Jul 29, 2017

View reviewed changes

codefromthecrypt reviewed Jul 29, 2017

View reviewed changes

ejona86 reviewed Jul 31, 2017

View reviewed changes

zpencer added 3 commits August 1, 2017 15:47

Fix Context.attach() to return storage ret val; use migration trick

7d5c4e6

add unit test for Context#detach asserting checkNotNull

433aca6

use existing executor

637039f

ejona86 requested changes Aug 2, 2017

View reviewed changes

codefromthecrypt reviewed Aug 4, 2017

View reviewed changes

zpencer added 2 commits August 4, 2017 11:25

Github comments

049fa3a

- update tests - make Context handle NULL values

fix comment, more checks

7fb5069

zhangkun83 reviewed Aug 4, 2017

View reviewed changes

expand javadoc to cover null return value from storage

859af19

zhangkun83 approved these changes Aug 4, 2017

View reviewed changes

codefromthecrypt reviewed Aug 5, 2017

View reviewed changes

zhangkun83 requested changes Aug 5, 2017

View reviewed changes

codefromthecrypt reviewed Aug 5, 2017

View reviewed changes

reflection unit test

e638c5b

zhangkun83 approved these changes Aug 7, 2017

View reviewed changes

codefromthecrypt approved these changes Aug 8, 2017

View reviewed changes

ejona86 approved these changes Aug 10, 2017

View reviewed changes

zpencer merged commit e195c1a into grpc:master Aug 11, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

context: change storage API to return a restoreable context #3292

context: change storage API to return a restoreable context #3292

zpencer commented Jul 28, 2017 •

edited

zhangkun83 Jul 29, 2017

zpencer Aug 1, 2017

zhangkun83 Jul 29, 2017

zpencer Aug 4, 2017

codefromthecrypt left a comment

codefromthecrypt commented Jul 30, 2017

ejona86 left a comment •

edited

ejona86 commented Jul 31, 2017 •

edited

codefromthecrypt commented Aug 1, 2017 via email

ejona86 Aug 2, 2017

zpencer Aug 4, 2017

ejona86 Aug 2, 2017

zpencer Aug 3, 2017 •

edited

codefromthecrypt Aug 4, 2017

zpencer Aug 4, 2017

zhangkun83 Aug 4, 2017

codefromthecrypt Aug 5, 2017

zpencer Aug 5, 2017 •

edited

zhangkun83 left a comment

codefromthecrypt Aug 5, 2017

zpencer commented Aug 9, 2017

zpencer commented Aug 11, 2017 •

edited

zpencer commented Aug 11, 2017

		@@ -16,6 +16,7 @@

		package io.grpc;

		import com.sun.istack.internal.Nullable;

context: change storage API to return a restoreable context #3292

context: change storage API to return a restoreable context #3292

Conversation

zpencer commented Jul 28, 2017 • edited

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

codefromthecrypt left a comment

Choose a reason for hiding this comment

codefromthecrypt commented Jul 30, 2017

ejona86 left a comment • edited

Choose a reason for hiding this comment

ejona86 commented Jul 31, 2017 • edited

codefromthecrypt commented Aug 1, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zpencer Aug 3, 2017 • edited

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

zpencer Aug 5, 2017 • edited

Choose a reason for hiding this comment

zhangkun83 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zpencer commented Aug 9, 2017

zpencer commented Aug 11, 2017 • edited

zpencer commented Aug 11, 2017

zpencer commented Jul 28, 2017 •

edited

ejona86 left a comment •

edited

ejona86 commented Jul 31, 2017 •

edited

zpencer Aug 3, 2017 •

edited

zpencer Aug 5, 2017 •

edited

zpencer commented Aug 11, 2017 •

edited