Skip to content

L112: Node Server Interceptors#406

Merged
murgatroid99 merged 10 commits intogrpc:masterfrom
murgatroid99:node_server_interceptors
Feb 1, 2024
Merged

L112: Node Server Interceptors#406
murgatroid99 merged 10 commits intogrpc:masterfrom
murgatroid99:node_server_interceptors

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
@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