L5: Add client interceptor support to NodeJS

[drobertduke]

No description provided.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[drobertduke]

I signed the CLA under Netflix

[googlebot]

CLAs look good, thanks!

[drobertduke]

@murgatroid99 @a11r

[murgatroid99]

I have created a grpc-io discussion about this proposal at https://groups.google.com/forum/#!topic/grpc-io/LxT1JjN33Q4. Please add a link to it in the proposal.

[ctiller]

Please rename to L5-something-something.md

[murgatroid99]

I'm sorry for the delay. I'm the maintainer for the Node gRPC library, so you should make me the reviewer listed in the doc. I am also approving this for submission, so you should also change the state to "Final".

[drobertduke]

We are exploring some minor changes to the interceptor API to bring it more in line with the Java interceptor framework. If they work out I will have an update to the proposal next week.

[murgatroid99]

Are you still planning to make changes to this API? Do you have an ETA for those changes?

[drobertduke]

Proposal has been updated with the API change. The new API allows interceptor developers to construct a new call at any time with all the downstream interceptors, which is important for the 'retry' use case.

[atian25]

+1 for this

[murgatroid99]

Also, @drobertduke can you make the previously mentioned changes to the information in the document's header?

[mehrdada]

@wenbozhu @stanley-cheung friendly ping... Can you please confirm this looks fine for gRPC-Web?

[mehrdada]

@wenbozhu friendly ping

[wenbozhu]

@drobertduke Could you briefly mention the difference between the Node proposal and the existing Java interceptor framework, if any, ignoring threading or language-specific design choices?

I don't see flow-control is addressed explicitly, i.e. pause/resume of Node streams. Are you using Node streams to manage flow-control today?

Re: the spec, I don't see how exceptions (from interceptors) are to be handled; or interceptors are allowed to terminate the request. Specifically, if an interceptor terminates a request, will (way-out) handlers of in-stack interceptors be invoked in order ... e.g. if an auth interceptor fails a request, will the outer logger interceptor sees an error response?

===

The overall proposal seems to me a rather low-level interceptor, which is useful for infrastructure or framework providers but may prove to be too complicated (powerful?) for normal users. This is just a note.

[drobertduke]

@wenbozhu
The Node proposal follows the InterceptingCall/ClientCall pattern used in grpc-java. Below I've implemented a trace-log interceptor in both Java and Node to illustrate the similarities/differences.

Java: https://gist.github.com/drobertduke/e48d08645ba872a933d192cd232bf3b7

NodeJS: https://gist.github.com/drobertduke/239e5668a20c8114626873886c754c9e

The listener methods have been renamed for clarity and there are minor interface differences but structurally they are very close. We have extensive interceptor implementations in Java and we need a NodeJS interceptor framework with a similar API to make re-implementation possible.

re: flow-control, before this PR NodeJS clients were using Node streams to manage flow control, and this PR does not change that. I'm not sure the flow control APIs exposed in grpc-java make sense when applied to Node streams but I'm happy to reconsider if there are specific requests for functionality.

re: exceptions, the framework follows the Joyent recommendation on exceptions, which is essentially 'Don't catch them unless you know they are operational errors'. Interceptor authors are expected to make their own determinations about handling exceptions.

re: terminating the request, interceptors can terminate the request in the same way Java interceptors terminate requests, and this causes 'outer' interceptors to be skipped (same as Java). Here's an example block which terminates a request when a client-side concurrency limit is reached:

if (inFlightRequests > concurrencyLimit) {
    return new grpc.InterceptingCall(null, {
        start: function(metadata, listener, next) {
            listener.onReceiveMetadata(new grpc.Metadata());
            listener.onReceiveMessage(undefined);
            listener.onReceiveStatus({code: RESOURCE_EXHAUSTED});
        }
    });
}

Should I make this more clear in the proposal?

Can you expand on what you mean by low-level? It follows a very similar API to grpc-java. A simpler API was considered but it would have ruled out the implementation of advanced functionality like client-side caching.

[wenbozhu]

@drobertduke

Thanks for the examples.

Re: flow-control, yes, I understand that the Java flow-control must be different than the pause/resume style of flow-control Node streams offers. I was asking if pause/resume need be intercepted too. Also, I am curious how pause/resume are actually used today, e.g. in your experience (independently of the interceptor design).

Re: exceptions, early termination, I think the spec should mention those conditions. Not sure if outer interceptors should be skipped, but this is not a Node specific issue.

Otherwise, LGTM.

Thanks.

[murgatroid99]

The flow control question brings up another one: for outgoing intercepted calls, how many times can each next callback be called? Some of your examples seem to result in some callbacks getting called zero times in some cases. Can they ever be called more than once? If so, particularly for messages, are the multiple sent messages properly flow-controlled?

[drobertduke]

@wenbozhu We're not using pause/resume on gRPC streams and I don't know of a use case for intercepting them. I've added some language to the spec to describe exception handling and the short-circuit behavior.

@murgatroid99 next should be called 0 or 1 times, calling it more than once is undefined behavior. I could add a requirement to throw an error when next is called twice for a single phase of a call if you think that should be in there.

[mehrdada]

@drobertduke If next cannot be called more than once, how does one accomplish the retry use-case?

[drobertduke]

@mehrdada To retry, create a new "call" using the interceptor's nextCall param, as seen here. next is for continuing an individual stage of a request/response, which doesn't work for retries. In a retry, the full RPC has already completed (from grpc-core's perspective), so we need a new "call" and we need to run every outbound phase on it from the beginning (start, sendMessage, halfClose).

This is similar to how we implement retries in Java. I don't see any open-source examples of retry interceptors but the next.newCall shown here is used similarly to the nextCall param in Javascript.

[mehrdada]

@drobertduke I believe the concrete use-case @wenbozhu particularly cares about is being able to defer sending initial headers to the server in streaming calls until after the first request message is seen by the interceptor and only then start the actual request on the wire, so that you are able to copy some fields from the request to the RPC headers within an interceptor.

[drobertduke]

There's an example of a unary interceptor for a similar case in the proposal: https://github.com/grpc/proposal/pull/14/files#diff-c1aee0ddae63a3e9a9ba050796cd4b58R279

The caching interceptor linked above delays sending the headers or message until the cache is checked. To satisfy your use case, you could store the start method's next and call it in sendMessage after modifying the metadata, delaying the transmission of headers. Here's a test case demonstrating that for both client-streaming RPC types. This test works if you add it to the PR branch for the implementation of this proposal:

  describe('delay streaming headers', function() {
    var foo_interceptor = function(options, nextCall) {
      var startNext;
      var startListener;
      var startMetadata;
      var methods = {
        start: function (metadata, listener, next) {
          startNext = next;
          startListener = listener;
          startMetadata = metadata;
        },
        sendMessage: function (message, next) {
          startMetadata.set('fromMessage', message.value);
          startNext(startMetadata, startListener);
          next(message);
        }
      };
      return new grpc_client.InterceptingCall(nextCall(options), methods);
    };
    var options = { interceptors: [foo_interceptor] };
    var metadata = new Metadata();
    it('with client streaming call', function(done) {
      var message = {value: 'foo'};
      var client_stream = client.echoClientStream(metadata, options, function() {});
      client_stream.on('metadata', function(metadata) {
        assert.equal(metadata.get('fromMessage'), 'foo');
        done();
      });
      client_stream.write(message);
      client_stream.end();
    });
    it('with bidi streaming call', function(done) {
      var message = {value: 'foo'};
      var bidi_stream = client.echoBidiStream(metadata, options);
      bidi_stream.on('metadata', function(metadata) {
        assert.equal(metadata.get('fromMessage'), 'foo');
        done();
      });
      bidi_stream.write(message);
      bidi_stream.end();
    });
  });

[mehrdada]

@drobertduke Thanks for the clarification.

[mehrdada]

@murgatroid99 @wenbozhu shall we merge this or are there any more questions?

[shinzui]

What's the status of this proposal?

[nicolasnoble]

Merged and implemented for a while now.

…

On Sat, Apr 28, 2018, 12:48 Nadeem Bitar ***@***.***> wrote: What's the status of this proposal? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#14 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AG8bpkqYIKqzuOIbYU3ystMKbH12uSZ3ks5ttMeogaJpZM4MJeCn> .