Don't hold a lock in DelayedStream when calling realStream #1526

ejona86 · 2016-03-05T20:26:10Z

Our currently lock ordering rules prohibit holding a lock when calling
the channel and stream. This change avoids the lock for both
DelayedClientTransport and DelayedStream. It is effectively a rewrite of
DelayedStream.

The fixes to ClientCallImpl were to ensure sane state in DelayedStream.

zhangkun83 · 2016-03-07T19:55:45Z

core/src/main/java/io/grpc/internal/DelayedStream.java

-    } else {
-      realStream.setAuthority(authority);
+    List<Runnable> toRun = new ArrayList<Runnable>();
+    while (true) {


This part is pretty tricky! It's worth a comment to explain why the loop is necessary. IIUC, it's because 1) runnable has to be run outside the lock; 2) passThrough can only be set to true when pendingCalls is empty, and this must be done inside the lock. So while runnable are running outside the lock, new runnables may be added to pendingCalls.

ejona86 · 2016-03-07T22:02:50Z

@zhangkun83, PTAL; now with some more comments

zhangkun83 · 2016-03-07T22:52:09Z

core/src/main/java/io/grpc/internal/DelayedStream.java

+      if (!cancelNow) {
+        error = reason;
+        listenerToClose = listener;
+        // TODO(ejona): Close InputStreams?


Do other stream implementations do so?

Currently MessageFramer doesn't. At present that doesn't hurt much, but we will need to fix places eventually.

ejona86 · 2016-03-07T23:45:08Z

@zhangkun83, @carl-mastrangelo, now with immediate calling of setDecompressor

zhangkun83 · 2016-03-08T00:00:12Z

core/src/main/java/io/grpc/internal/DelayedStream.java

-    startedRealStream.setDecompressor(decompressor);
+  public void setDecompressor(final Decompressor decompressor) {
+    checkNotNull(decompressor, "decompressor");
+    checkState(realStream != null, "How did we receive a reply before the request is sent?");


Here realStream is accessed from a thread different from what it is set. Shouldn't realStream be volatile?
Or check passThrough first?

This is only expected to be called after setStream, since this is for processing responses. If setDecompressor is called in the same thread as setStream, then there are no memory concerns. If it is called in a different thread then that means there was some lock/volatile after setStream in order to transfer work to that other thread.

Please correct me if I misunderstood Java memory model. My understanding is, when a work is transferred across threads locally, there is always some lock/volatile which form memory barrier between the write on the first thread and the read on the second thread. In this case, although logically setStream() "happens before" setDecompressor(), there is a network round-trip in the middle. I doubt Java memory model could form a memory barrier for events across network.

... Good point. I can't find much to define what happens due to I/O. I've added an empty synchronized block to trigger happens-before.

zhangkun83 · 2016-03-08T19:26:37Z

LGTM

carl-mastrangelo · 2016-03-08T19:37:55Z

core/src/main/java/io/grpc/internal/DelayedStream.java

+      if (passThrough) {
+        runNow = true;
+      } else {
+        pendingCalls.add(runnable);


Why not just return after this point, and not check runNow at the end of this func?

ejona86 · 2016-03-08T20:36:15Z

@carl-mastrangelo, PTAL

carl-mastrangelo · 2016-03-08T20:52:25Z

core/src/main/java/io/grpc/internal/DelayedStream.java

+      delayOrExecute(new Runnable() {
+        @Override
+        public void run() {
+          realStream.flush();


This loses the original codes intent. Flushes were coalesced previously. Are you certain that is the behavior you want?

I was okay with replaying each flush. I wasn't too concerned about that level of performance in the slow case. It should be possible to coalesce flushes, but is slightly non-trivial because of interactions with other methods like halfClose() and cancel(). I'm don't have much problem calling optimizing flush as future work, but I do have some trouble caring about the case at present because it will be no worse than when DelayedStream is not being used.

Ok, just wanted to make sure this was intentional, not a side effect.

ejona86 · 2016-03-09T23:06:57Z

@carl-mastrangelo, thanks for the comments. PTAL

carl-mastrangelo · 2016-03-09T23:10:44Z

@ejona86 LGTM

Our current lock ordering rules prohibit holding a lock when calling the channel and stream. This change avoids the lock for both DelayedClientTransport and DelayedStream. It is effectively a rewrite of DelayedStream. The fixes to ClientCallImpl were to ensure sane state in DelayedStream. Fixes grpc#1510

ejona86 assigned zhangkun83 Mar 5, 2016

zhangkun83 reviewed Mar 7, 2016
View reviewed changes

ejona86 force-pushed the delayed-no-lock-when-calling-stream branch from 2cd1b3a to a5fa38c Compare March 7, 2016 22:01

zhangkun83 reviewed Mar 7, 2016
View reviewed changes

ejona86 force-pushed the delayed-no-lock-when-calling-stream branch from 0bcdd7b to cde9fb8 Compare March 7, 2016 23:44

zhangkun83 reviewed Mar 8, 2016
View reviewed changes

carl-mastrangelo reviewed Mar 8, 2016
View reviewed changes

ejona86 force-pushed the delayed-no-lock-when-calling-stream branch from 7ac08c4 to eccd231 Compare March 9, 2016 23:15

ejona86 merged commit eccd231 into grpc:master Mar 9, 2016

ejona86 deleted the delayed-no-lock-when-calling-stream branch March 9, 2016 23:38

zhangkun83 mentioned this pull request Mar 11, 2016

Only link delayed transport AFTER real transport has called transportReady() #1494

Merged

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

Don't hold a lock in DelayedStream when calling realStream #1526

Don't hold a lock in DelayedStream when calling realStream #1526

Uh oh!

Conversation

ejona86 commented Mar 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ejona86 commented Mar 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ejona86 commented Mar 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhangkun83 commented Mar 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ejona86 commented Mar 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ejona86 commented Mar 9, 2016

Uh oh!

carl-mastrangelo commented Mar 9, 2016

Uh oh!

Uh oh!