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

L112: Node Server Interceptors #406

Merged
merged 10 commits into from
Feb 1, 2024

Conversation

murgatroid99
Copy link
Member

No description provided.

method handler
```

## Rationale
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a section on how closely (or not) the above APIs match the client-side interceptor design & apis?

Copy link
Member Author

Choose a reason for hiding this comment

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

You can see the client interceptor APIs in https://github.com/grpc/proposal/blob/master/L5-node-client-interceptors.md. They are as close as possible, but they're not identical because there are inherent differences in client and server functionality. I said that the design "mirrors" the client interceptor design in the abstract, in the sense that client send operations generally correspond to server receive operations, and vice versa.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a section to show the correspondence.


### Single injection point

The client interceptor design has interceptor lists and interceptor provider lists, and interceptors specified at client construction vs interceptors specified for individual call invocations. In contrast, this design only specifies that an interceptor list can be provided at server construction. The primary purpose of this design is to solidify the interceptor part of the design. Additional injection points for interceptors could be specified in the future, and the way they interact with each other can be decided at that time.
Copy link
Member

Choose a reason for hiding this comment

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

This is probably one of the areas that is unique to server interceptors - which by design shouldn't support per-call inceptor injection as any inbound operations/events are supposed to be intercepted before any business logic is invoked (as part of the service method implementation).

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated this section to try to clarify it.

The `Server` constructor's `options` parameter will accept a new option `interceptors`, which is an array of interceptor functions.


#### Interceptor nesting
Copy link
Member

Choose a reason for hiding this comment

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

Nit: chaining may be more commonly used for this term.

One of the reasons I asked about the client interceptor design is to find out if I need to review the interceptor chaining or nesting flow.

E.g. if interceptorA onReceiveMetadata() decides to respond a status immediately (e.g. an auth failure) will interceptorB/C onReceiveMetadata() be called by the framework or the call stack will jump to the outbound flow (for interceptorA).

Copy link
Member Author

Choose a reason for hiding this comment

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

I used "nesting" because it's the term that the corresponding section of the client interceptors design used.

The next callback in each method allows the interceptor author to control that flow. In your particular auth failure example, the auth interceptor's onReceiveMetadata method can explicitly send a non-OK status and not call the next callback. As a result, the entire rest of the inbound/outbound flow will be skipped, and the handler will never be invoked. This is broadly the same as in the client interceptors design.

Copy link
Member

@wenbozhu wenbozhu left a comment

Choose a reason for hiding this comment

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

LGTM

I left a comment on interceptor nesting. If any design changes are needed, they can be done separately.

@wenbozhu
Copy link
Member

wenbozhu commented Jan 8, 2024

LGTM. Thanks.

@wenbozhu wenbozhu closed this Jan 8, 2024
@murgatroid99 murgatroid99 reopened this Jan 8, 2024
@murgatroid99
Copy link
Member Author

I made a few changes to address issues that came up while writing the implementation.

@murgatroid99 murgatroid99 merged commit 8d02271 into grpc:master Feb 1, 2024
1 check passed
@msharbaji
Copy link

@murgatroid99 Is this proposal under implementation?

@murgatroid99
Copy link
Member Author

The implementation of this proposal is available in grpc-js version 1.10.x

@msharbaji
Copy link

@murgatroid99 Thanks for letting me know :)

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.

3 participants