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

Ability to use a single interceptor for both unary and streaming RPCs #1805

Open
jhump opened this issue Jan 17, 2018 · 8 comments
Open

Ability to use a single interceptor for both unary and streaming RPCs #1805

jhump opened this issue Jan 17, 2018 · 8 comments
Assignees
Labels

Comments

@jhump
Copy link
Member

@jhump jhump commented Jan 17, 2018

After #1801 is completed, it becomes possible for a stream interceptor to be used when processing unary RPCs.

This issue is a feature request for some new API that allows for exactly this. The existing API, for backwards compatibility, will continue to register a stream interceptor that is only used for streaming RPCs. But a new method/dial option could be added that allows for registering a stream interceptor that is used for unary RPCs and/or for both streaming and unary RPCs.

@dfawley
Copy link
Contributor

@dfawley dfawley commented Mar 15, 2018

I'd like to redesign our interceptor API. Please add your list of requirements/nice-to-haves in this bug and I'll compile them here:

Requirements:

  • Single interceptor type for unary and streaming
    • If it's difficult to implement, provide an optional convenience wrapper to generate a stream-style interceptor from a single function (like how unary interceptors work today).
  • Include access to ServiceDesc/FileDescriptorProto (#1526)
  • Chainable / able to easily register multiple handlers

Nice to Haves:

  • Single API for stats handlers and interceptors
  • Global registration pattern
  • Able to specify via CallOptions
  • Function signature includes explicit context.Context (from @skipor)
  • Same/similar type signature as RPC handler itself (Java does this)
  • Include [de]serialization steps as interceptors to allow inserting interceptors before deserialization or after serialization.

This is currently just a quick list based on my memory at the moment. I intend to add to this as I run into more things.

This will be a breaking change (breaking only features marked "EXPERIMENTAL" in the docs), but we will make sure to leave the current version around for one or two releases to allow time for users to migrate. We will also be sure to make a gRFC proposal for this. We will maintain the existing interceptors - two of the four were not marked as experimental and they are relied heavily enough upon that it is not feasible to remove them.

@skipor
Copy link

@skipor skipor commented Apr 24, 2018

Nice to Have, for Interceptors redesign: extract context.Context from ServerStream and pass it as the first argument in stream interceptor.
Now every context.Context wrapping requires also ServerStream wrapping, which is too complex. In my experience Context wrapping with value or timeout is a very common pattern, and it would be really nice to use it as everywhere else.

@devpro108
Copy link

@devpro108 devpro108 commented Jun 26, 2019

@dfawley wondering any news on new interceptor design, when it will be available, looking forward to single interceptor for both unary and streams..

@dfawley
Copy link
Contributor

@dfawley dfawley commented Jun 27, 2019

For the moment, there are no plans to work on this, as there are just too many other, higher-priority things on our plate. However, when investigating how to support the xDS protocol for gRPC-LB v2 (ref), it's appearing that interceptors could play a prominent role. If that turns out to be the case, we may be able to do some or all of this work as part of that effort.

@devpro108
Copy link

@devpro108 devpro108 commented Jul 17, 2019

Thank you @dfawley

@stale
Copy link

@stale stale bot commented Sep 6, 2019

This issue is labeled as requiring an update from the reporter, and no update has been received after 7 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@stale stale bot added the stale label Sep 6, 2019
@jhump
Copy link
Member Author

@jhump jhump commented Sep 6, 2019

@dfawley, I think stale bot got it wrong on this one, too. I see this is "on hold", but probably should not be closed -- AFAICT, it is not pending any further input from the reporter(s).

@stale stale bot removed the stale label Sep 6, 2019
@dfawley
Copy link
Contributor

@dfawley dfawley commented Sep 6, 2019

Yeah, it hit a lot of issues this morning. It doesn't seem to be listening to the "onlyLabels" setting (or we set it wrong).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants