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

fix: call ResponseMetadataHanlder#onTrailers before calling onClose #1549

Merged
merged 1 commit into from Nov 3, 2021

Conversation

mutianf
Copy link
Contributor

@mutianf mutianf commented Nov 2, 2021

metadataHandler.onTrailers(..) should be called before onClose(..). With the current implementation, I don't think it's possible for ServerStreamingCallables or UnaryCallable#futureCalls() to get the trailers because the trailers are set after the call is closed?

The comment in the code and test case also only showed synchronized calls for UnaryCallables.

Use a server stream call as an example, the way I'm trying to access the trailer is:

class MyStreamingCallable<RequestT, ResponseT> extends ServerStreamingCallable  {
  @Override
  public void call(RequestT request, ResponseObserver<ResponseT> responseObserver, ApiCallContext context) {
      final GrpcResponseMetadata responseMetadata = new GrpcResponseMetadata();
      MyResponseObserver<ResponseT> innerObserver = new MyResponseObserver<>(responseObserver, responseMetadata);
      ApiCallContext withResponseMetadata = responseMetadata.addHandlers(context);
      innerCallable.call(request, innerObserver, withResponseMetadata);
  }

  // Response observer class
  private class MyResponseObserver<ResponseT> implements ResponseObserver<ResponseT> {
    @Override
    public void onComplete() {
      // With the current implementation, trailers will be null because we're still calling #onClose()
      Metadata trailers = responseMetadata.getTrailingMetadata();
      ...
      outerObserver.onComplete();
    }

    // onError will be similar to onComplete
  }
}

Unless GrpcResponseMetadata is not intended to be used this way?

@mutianf mutianf requested review from as code owners Nov 2, 2021
@google-cla google-cla bot added the cla: yes label Nov 2, 2021
@chanseokoh
Copy link
Contributor

@chanseokoh chanseokoh commented Nov 2, 2021

If you want to get the trailer on an event, I guess you can just extend GrpcResponseMetadata, which implements ResponseMetadataHandler.onTrailers()? Or, implement ResponseMetadataHandler.onTrailers() yourself?

But to be fair, I'm not actually familiar with this.

@mutianf
Copy link
Contributor Author

@mutianf mutianf commented Nov 2, 2021

If you want to get the trailer on an event, I guess you can just extend GrpcResponseMetadata, which implements ResponseMetadataHandler.onTrailers()? Or, implement ResponseMetadataHandler.onTrailers() yourself?

But to be fair, I'm not actually familiar with this.

Yes, it shouldn't be hard to get the trailers. I guess I'm proposing this change because I thought GrpcResponseMetadata gives client libraries a nice and easy way to access headers and trailers, so we won't need to implement our own versions of interceptors etc in the client libraries. But with the current implementation I'm not sure how I can get the trailers easily 😢 which defeats the point of providing a ResponeHandler class.

Copy link
Contributor

@vam-google vam-google left a comment

LGTM

@igorbernstein2 igorbernstein2 merged commit 19a77a4 into googleapis:main Nov 3, 2021
5 checks passed
@mutianf mutianf deleted the metadata_handler branch Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants