Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RATIS-920. Fix error use of onCompleted in onError #87

Closed
wants to merge 2 commits into from
Closed

RATIS-920. Fix error use of onCompleted in onError #87

wants to merge 2 commits into from

Conversation

runzhiwang
Copy link
Contributor

@runzhiwang runzhiwang commented May 5, 2020

Apache jira:
https://issues.apache.org/jira/browse/RATIS-920

What's the problem ?
When run test, a lot of StatusRuntimeException: CANCELLED: call already cancelled was thrown, as the image shows.
image

What's the reason ?

  1. When run into onError, the call has already been cancelled, there is no need to call onCompleted. You can find from the following doc of onError, it's the last method call, and
    no further calls to any method are allowed.

void onError(Throwable t)
Receives a terminating error from the stream.
May only be called once and if called it must be the last method called. In particular if an exception is thrown by an implementation of onError no further calls to any method are allowed.

t should be a StatusException or StatusRuntimeException, but other Throwable types are possible. Callers should generally convert from a Status via Status.asException() or Status.asRuntimeException(). Implementations should generally convert to a Status via Status.fromThrowable(Throwable).

  1. You can find why the exception was thrown in the following code, copied from gRPC.

2.1. Before call requestObserver.onError, it will first set responseObserver.cancelled = true. Besides the onCancelHandler is null, because it was not set in ratis.

      @Override
      public void onCancel() {
        responseObserver.cancelled = true;
        if (responseObserver.onCancelHandler != null) {
          responseObserver.onCancelHandler.run();
        }
        if (!halfClosed) {
          requestObserver.onError(
              Status.CANCELLED
                  .withDescription("cancelled before receiving half close")
                  .asRuntimeException());
        }
      }

2.2. When call responseObserver.onCompleted, because cancelled == true and onCancelHandler == null, then exception was threw.

    @Override
    public void onCompleted() {
      if (cancelled) {
        if (onCancelHandler == null) {
          throw Status.CANCELLED.withDescription("call already cancelled").asRuntimeException();
        }
      } else {
        call.close(Status.OK, new Metadata());
        completed = true;
      }
    }

@runzhiwang runzhiwang changed the title RATIS-920. Fix error use of onCompleted in onError [WIP]RATIS-920. Fix error use of onCompleted in onError May 5, 2020
@runzhiwang runzhiwang changed the title [WIP]RATIS-920. Fix error use of onCompleted in onError RATIS-920. Fix error use of onCompleted in onError May 7, 2020
@runzhiwang
Copy link
Contributor Author

@lokeshj1703 Could you help review this patch ? Thank you very much.

@lokeshj1703
Copy link
Contributor

@runzhiwang Thanks for working on this! I think this scenario will occur when client cancels the call. How about we extract the status in onError(using Status.fromThrowable(Throwable)) and check if it is cancelled. If it is cancelled we do not make the onCompleted call. Let's see if that solves the problem.

@runzhiwang
Copy link
Contributor Author

@lokeshj1703 Thanks for review. The suggestion does work. I have updated.

Copy link
Contributor

@lokeshj1703 lokeshj1703 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@runzhiwang Thanks for updating the PR! The changes look good to me. +1.

@lokeshj1703
Copy link
Contributor

@runzhiwang Thanks for the contribution! I have committed the PR to master branch.

@lokeshj1703 lokeshj1703 closed this May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants