core: change serverimpl,servercallimpl's internalclose to cancel stream #4038

ramaraochavali · 2018-02-04T09:45:36Z

Signed-off-by: Rama [email protected]
Fixes #3819, #3746

ejona86 · 2018-02-15T00:32:57Z

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

@@ -205,7 +205,7 @@ public String getAuthority() {
   * on.
   */
  private void internalClose(Status internalError) {
-    stream.close(internalError, new Metadata());
+    stream.cancel(internalError);


This status is stored internally in gRPC, but won't actually be exposed anywhere on server-side. We should probably go ahead and log it here.

ok. which level? Is SEVERE the correct level? I have added with WARN. Let me know if needs to be changed.

WARNING seems good. SEVERE seems a bit too much, because we were able to recover.

ejona86 · 2018-02-15T00:42:47Z

interop-testing/src/test/java/io/grpc/testing/integration/MoreInProcessTest.java

@@ -247,7 +247,7 @@ public void onCompleted() {
        .onNext(StreamingInputCallRequest.getDefaultInstance());

    assertTrue(finishLatch.await(900, TimeUnit.MILLISECONDS));
-    assertEquals(Status.UNKNOWN, Status.fromThrowable(throwableRef.get()));
+    assertEquals(Status.CANCELLED.getCode(), Status.fromThrowable(throwableRef.get()).getCode());


Right now calling serverStream.cancel() will propagate a CANCEL HTTP/2 error to the remote. It'd probably be better if it was INTERNAL_ERROR, which will get mapped to a INTERNAL grpc status code. That way the client would get a signal that it shouldn't retry.

I think it'd be fair to map gRPC's DEADLINE_EXCEEDED and CANCELLED error codes to HTTP/2's CANCEL, and everything else map to HTTP/2's INTERNAL_ERROR. Right now we use DEADLINE_EXCEEDED for when deadline expires. That mapping could be in NettyServerHandler.cancelStream().

jyane · 2018-02-15T03:48:00Z

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

@@ -610,8 +610,7 @@ void setListener(ServerStreamListener listener) {
     * Like {@link ServerCall#close(Status, Metadata)}, but thread-safe for internal use.
     */
    private void internalClose() {
-      // TODO(ejona86): this is not thread-safe :)
-      stream.close(Status.UNKNOWN, new Metadata());
+      stream.cancel(Status.INTERNAL);


~~Fix doc comments if you change?~~ edit: Please don't care.

…ncelStream Signed-off-by: Rama <[email protected]>

ramaraochavali · 2018-02-16T07:07:38Z

@ejona86 addressed the comments, PTAL and let me know if any further changes are needed.

Signed-off-by: Rama <[email protected]>

ejona86 · 2018-02-20T22:16:40Z

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

@@ -205,7 +207,8 @@ public String getAuthority() {
   * on.
   */
  private void internalClose(Status internalError) {
-    stream.close(internalError, new Metadata());
+    log.log(Level.WARNING,format("cancelling the stream with status %s",internalError.toString()));


Pass internalError as an argument and use MessageFormat-style syntax:

log.log(Level.WARNING, "Cancelling the stream with status {0}", new Object[] {internalError});

We mainly use String.format() when we need to create a gRPC status description with an argument.

ejona86 · 2018-02-20T22:19:39Z

One small change, and then I can merge.

…rmat Signed-off-by: Rama <[email protected]>

ramaraochavali · 2018-02-21T04:05:35Z

@ejona86 changed. PTAL.

…ose to cancel stream (grpc#4038)" This partially reverts commit 48ca452. It leaves the changes to ServerCallImpl and test. This also partially reverts "Lint fixes" commit 3002a23 which removed unused variables which are now necessary again.

…ose to cancel stream (grpc#4038)" This partially reverts commit 48ca452. It leaves the changes to ServerCallImpl and test. This also partially reverts "Lint fixes" commit 3002a23 which removed unused variables which are now necessary again. This is reverted for the combined result of two issues: * Some users are testing that they get UNKNOWN when the service throws. That's not unreasonable given the behavior was well-publicised when it changed in v1.5. We should probably keep the UNKNOWN in some common cases (like the service threw immediately, before sending anything). * The client always saw CANCELLED, not INTERNAL as had been intended. This is behavior is visible in the tests and was overlooked during reveiw.

…ose to cancel stream (grpc#4038)" This partially reverts commit 48ca452. It leaves the changes to ServerCallImpl and test. This also partially reverts "Lint fixes" commit 3002a23 which removed unused variables which are now necessary again. This is reverted for the combined result of two issues: * Some users are testing that they get UNKNOWN when the service throws. That's not unreasonable given the behavior was well-publicised when it changed in v1.5. We should probably keep the UNKNOWN in some common cases (like the service threw immediately, before sending anything). * The client always saw CANCELLED, not INTERNAL as had been intended. This is behavior is visible in the tests and was overlooked during review.

…ose to cancel stream (grpc#4038)" This partially reverts commit 48ca452. It leaves the changes to ServerCallImpl and test. This also partially reverts "Lint fixes" commit 3002a23 which removed unused variables which are now necessary again. This is reverted for the combined result of two issues: * Some users are testing that they get UNKNOWN when the service throws. That's not unreasonable given the behavior was well-publicised when it changed in v1.5. We should probably keep the UNKNOWN in some common cases (like the service threw immediately, before sending anything). * The client could see CANCELLED instead of INTERNAL as had been intended. It's unclear as to why (I didn't investigate heavily). This behavior is visible in MoreInProcessTest and was overlooked during review.

…ose to cancel stream (#4038)" This partially reverts commit 48ca452. It leaves the changes to ServerCallImpl and test. This also partially reverts "Lint fixes" commit 3002a23 which removed unused variables which are now necessary again. This is reverted for the combined result of two issues: * Some users are testing that they get UNKNOWN when the service throws. That's not unreasonable given the behavior was well-publicised when it changed in v1.5. We should probably keep the UNKNOWN in some common cases (like the service threw immediately, before sending anything). * The client could see CANCELLED instead of INTERNAL as had been intended. It's unclear as to why (I didn't investigate heavily). This behavior is visible in MoreInProcessTest and was overlooked during review.

ramaraochavali added 2 commits February 4, 2018 15:11

core: change serverimpl,servercallimpl's internalclose to cancel stream

4787ccc

Signed-off-by: Rama <[email protected]>

test: fixed MoreInProcessTest

f64b5a4

Signed-off-by: Rama <[email protected]>

zhangkun83 requested a review from ejona86 February 7, 2018 18:45

ejona86 reviewed Feb 15, 2018

View reviewed changes

jyane reviewed Feb 15, 2018

View reviewed changes

core,netty: logged Status in cancel,changed status code mapping in ca…

4b99378

…ncelStream Signed-off-by: Rama <[email protected]>

netty: fixed equality condition

f08db82

Signed-off-by: Rama <[email protected]>

ejona86 approved these changes Feb 20, 2018

View reviewed changes

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Feb 20, 2018

kokoro-team removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Feb 20, 2018

core: changed the log call to use message format instead of string fo…

6c54490

…rmat Signed-off-by: Rama <[email protected]>

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Feb 21, 2018

kokoro-team removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Feb 21, 2018

ejona86 approved these changes Feb 21, 2018

View reviewed changes

ejona86 requested a review from zpencer February 21, 2018 16:38

zpencer approved these changes Feb 22, 2018

View reviewed changes

zpencer merged commit 48ca452 into grpc:master Feb 22, 2018

ramaraochavali deleted the fix/stream_close branch February 23, 2018 06:12

ejona86 mentioned this pull request Mar 2, 2018

Partially revert "core: change serverimpl,servercallimpl's internalcl… #4168

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: change serverimpl,servercallimpl's internalclose to cancel stream #4038

core: change serverimpl,servercallimpl's internalclose to cancel stream #4038

ramaraochavali commented Feb 4, 2018

ejona86 Feb 15, 2018

ramaraochavali Feb 16, 2018 •

edited

Loading

ejona86 Feb 16, 2018

ejona86 Feb 15, 2018

jyane Feb 15, 2018

ramaraochavali commented Feb 16, 2018

ejona86 Feb 20, 2018

ejona86 commented Feb 20, 2018

ramaraochavali commented Feb 21, 2018

core: change serverimpl,servercallimpl's internalclose to cancel stream #4038

core: change serverimpl,servercallimpl's internalclose to cancel stream #4038

Conversation

ramaraochavali commented Feb 4, 2018

ejona86 Feb 15, 2018

Choose a reason for hiding this comment

ramaraochavali Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

ejona86 Feb 16, 2018

Choose a reason for hiding this comment

ejona86 Feb 15, 2018

Choose a reason for hiding this comment

jyane Feb 15, 2018

Choose a reason for hiding this comment

ramaraochavali commented Feb 16, 2018

ejona86 Feb 20, 2018

Choose a reason for hiding this comment

ejona86 commented Feb 20, 2018

ramaraochavali commented Feb 21, 2018

ramaraochavali Feb 16, 2018 •

edited

Loading