Support for client interceptors

[rebello95]

I've written up a proposal to add support for client interceptors here: https://github.com/grpc/grpc-swift/wiki/Interceptors-Proposal (feedback welcome!)

This issue should serve to track the status of that proposal and its implementation.

[MrMage]

I like the concept! It reminds me of their server-side equivalent, which are called Middlewares in Swift-Vapor. In fact, my server-side request handlers are wrapped in a quick-and-dirty "middleware" call to log errors (just for illustration purposes):

fileprivate func runAndLogErrors<T>(_ session: ServerSession, _ work: (RequestContext) throws -> T) throws -> T {
	let context = RequestContext()
	context.session = session
	context.clientVersion = session.requestMetadata["client-version"]
	do {
		// Do some authentication...
		return try work(context)
	} catch {
		JSONLogger.sharedInstance.log(.from(
			error, context: context, session: session, source: .capture()))
		throw error
	}
}

A few remarks:

1. Why is Interceptor.intercept static?
2. Note that (I think) the CallResult is only available after all responses have been sent for server-streaming calls, not after the first one. I'm also not quite sure how you would handle client-streaming calls?
3. Did you consider an interface like the following instead: static func intercept(_ call: InterceptableCall, doneCallback: @escaping (CallResult) -> Void)? As far as I can tell, most of our current API (at least the asynchronous parts, i.e. everything except the blocking variant of unary calls) expects a callback to be executed when the call is finished, anyway. The interceptor would then call call.next(doneCallback: doneCallback), or something like call.next { response in /* mess with response */; doneCallback(response) }.
4. I think SwiftNIO has a similar concept by chaining several ChannelHandlers, which make injecting interceptors in between them fairly easy (I think). Given that we plan to migrate to (or at least add an alternative implementation based on) https://github.com/apple/swift-nio-http2 in the medium term, it might become much easier to implement such interceptors on that.

[MrMage]

P.S.: not sure when SwiftNIO support for iOS is going to materialize, though, so this definitely has merit for the current implementation as well.

[rebello95]

Thanks for the feedback @MrMage!

Item 1
Since the static variant of the Interceptor was synchronous, making a non-static version would require us to either a) have one interceptor instance handle every API call for a channel, or b) instantiate an interceptor for each call. I figured there wouldn't be much benefit to the latter in this case due to the fact that it's only handling one synchronous function. If this were async, I think we'd likely want to instantiate an interceptor for each request. What do you think?

Item 2
That's a good callout. I think this would still be okay for unary requests, but the lack of a CallResult until the end of a streamed request would be a problem. This could potentially be solved by sending a different type with the current data to interceptors instead of an explicit CallResult. Server streaming interceptors would be called multiple times (which would require async). For client streaming, maybe we could do something similar by calling interceptors multiple times as new data is sent up? I also added more details below.

Item 3
Under "Alternatives" I had something similar: func handleResponse(_ result: CallResult) -> CallResult. However, I really like your suggestion of using a doneCallback instead of a return value! Especially since it allows interceptors to do something async with the response if needed (such as refreshing a token, etc.).

Personally, I like the async approach to interceptors as opposed to the synchronous route. The primary reason I targeted a synchronous implementation was to try to provide a similar interface to what other gRPC language implementations do (like Java/PHP). I did notice that Go seems to have a different version that allows for multiple callbacks as streams continue, so it doesn't seem like consistency is a huge concern. If we're okay with being a little different, I'm definitely up for having Swift use an async approach and calling the interceptors as parts of RPC streams are sent/received. I can also update the proposal to flesh out the async design based on what's already there and the discussion from above if that's the direction we want to take. Thoughts? (Would also like to see what @timburks thinks of this.)

Item 4
Wasn't aware that SwiftNIO integration was on the roadmap, but that's an interesting idea for sure. I think it'd be great to build an interceptor system within gRPC, which could 1) work without the SwiftNIO flavor and b) act as an abstraction layer over SwiftNIO if/when that becomes an integration option.

[MrMage]

Item 1
Since the static variant of the Interceptor was synchronous, making a non-static version would require us to either a) have one interceptor instance handle every API call for a channel, or b) instantiate an interceptor for each call. I figured there wouldn't be much benefit to the latter in this case due to the fact that it's only handling one synchronous function. If this were async, I think we'd likely want to instantiate an interceptor for each request. What do you think?

I think there are good reasons where you'd e.g. like to pass a parameter to an Interceptor, so I think you should be able to pass an Interceptor instance to the channel constructor. One Interceptor per channel is probably fine; I don't see why it should be different for each request.

Item 2
That's a good callout. I think this would still be okay for unary requests, but the lack of a CallResult until the end of a streamed request would be a problem. This could potentially be solved by sending a different type with the current data to interceptors instead of an explicit CallResult. Server streaming interceptors would be called multiple times (which would require async). For client streaming, maybe we could do something similar by calling interceptors multiple times as new data is sent up? I also added more details below.

I guess that would work, just wanted to point it out. Would you mind elaborating the kind of work Interceptors could perform on the request/response protos that are piped through them? At the moment, it feels like the proposal only describes intercepting call results, but not actual data.

This makes me start to wonder whether we should instead indeed have a per-request interceptor (created e.g. via an interceptor factory) with an interface similar to this (just a very rough sketch):

protocol Interceptor {
associatedtype RequestType: Message
associatedtype ResponseType: Message
init?()  // when returning nil, the request should not be handled by this interceptor? (just an idea)
func onStart(method, metadata, completion: (method, metadata) -> Void)
func onRequest(request: RequestType, completion: (RequestType) -> Void) // called for each request message, gives us the option to manipulate any data sent by the client before it is passed to the network
func onResponse(response: RequestType, completion: (ResponseType) -> Void) // called for each response message, gives us the option to manipulate any data sent by the server before it is passed to the client
func onStatus(status: ServerStatus, completion: (ServerStatus) -> Void) // called once a status has been received
}

Some notes on this:

1. We could even split that out into different interceptor types for request creation, sending, receiving, and status. Or we'd have a default implementation for the on handlers that just calls through to completion.
2. We could also allow changing the types of requests, but that's probably going a bit far.
3. For what purposes exactly do you need this? Having the purpose in mind would help with designing a proper API. Like, do you need to intercept all requests, or just specific methods? And what do your planned interceptors do?
4. Let me know if this needs further explanation.

Item 3
Under "Alternatives" I had something similar: func handleResponse(_ result: CallResult) -> CallResult. However, I really like your suggestion of using a doneCallback instead of a return value! Especially since it allows interceptors to do something async with the response if needed (such as refreshing a token, etc.).

Personally, I like the async approach to interceptors as opposed to the synchronous route. The primary reason I targeted a synchronous implementation was to try to provide a similar interface to what other gRPC language implementations do (like Java/PHP). I did notice that Go seems to have a different version that allows for multiple callbacks as streams continue, so it doesn't seem like consistency is a huge concern. If we're okay with being a little different, I'm definitely up for having Swift use an async approach and calling the interceptors as parts of RPC streams are sent/received. I can also update the proposal to flesh out the async design based on what's already there and the discussion from above if that's the direction we want to take. Thoughts? (Would also like to see what @timburks thinks of this.)

I think in this case, async Interceptors with a callback are fine. We essentially just move from returning a result to sending the result in a callback, so it's not hugely different. We just need to document that the callback MUST be called.

Item 4
Wasn't aware that SwiftNIO integration was on the roadmap, but that's an interesting idea for sure. I think it'd be great to build an interceptor system within gRPC, which could 1) work without the SwiftNIO flavor and b) act as an abstraction layer over SwiftNIO if/when that becomes an integration option.

The SwiftNIO implementation will most likely entirely be different from the current implementation, as that one is very much based on gRPC-Core and its approach. On the other hand, SwiftNIO for iOS is uncertain, so we would probably get by without interceptors in the NIO-based implementation early on anyway.

[rebello95]

Item 1

I think there are good reasons where you'd e.g. like to pass a parameter to an Interceptor, so I think you should be able to pass an Interceptor instance to the channel constructor. One Interceptor per channel is probably fine; I don't see why it should be different for each request.

Sure, that's fair. My main concern with having a single interceptor instance per channel is that I've seen usages where the interceptor itself maintains some state based on the requests it's passed. Simple example of where having a single interceptor instance could be difficult:

final class MyInterceptor {
    private var analyticsEvent: SomeEvent?

    func handleRequest(...) {
        self.analyticsEvent = ...
    }

    func handleResponse(...) {
        self.analyticsEvent.update(...)
        // If this instance handles multiple requests, how do we track this one piece of state?
    }
}

Item 2
I had considered the idea of using generics or associated types (as you proposed) to allow request-specific interceptors, but it didn't really seem to fit the common use cases. To answer your question as to where these would be used, here are a few examples of how our team uses interceptors with HTTP:

• Injecting headers on all requests
• Holding back all outgoing requests while an OAuth token is refreshed, then starting all pending requests once we have a new token
• Collecting analytics on request/response sizes
• Analyzing failure rates and p99 on API calls

Based on these and my understanding of how interceptors in other languages have been implemented, it probably makes sense to stick to the catch-all approach for now though it could definitely be feasible to add request-specific interceptors if needed. Regarding the Data passed back, the above cases don't really need to know its type, but in the case of request-specific interceptors that could be useful. Do you have some cases in mind where this would be beneficial?

We could even split that out into different interceptor types for request creation, sending, receiving, and status

Could you add some examples of how this would be used? Trying to visualize it a bit better.

Items 3-4 make total sense 👍

[MrMage]

Sure, that's fair. My main concern with having a single interceptor instance per channel is that I've seen usages where the interceptor itself maintains some state based on the requests it's passed. Simple example of where having a single interceptor instance could be difficult:

Agreed, that's why I backtracked on that in my reply to item 2 :-)

I had considered the idea of using generics or associated types (as you proposed) to allow request-specific interceptors, but it didn't really seem to fit the common use cases. To answer your question as to where these would be used, here are a few examples of how our team uses interceptors with HTTP:

Injecting headers on all requests
Holding back all outgoing requests while an OAuth token is refreshed, then starting all pending requests once we have a new token
Collecting analytics on request/response sizes
Analyzing failure rates and p99 on API calls
Based on these and my understanding of how interceptors in other languages have been implemented, it probably makes sense to stick to the catch-all approach for now though it could definitely be feasible to add request-specific interceptors if needed. Regarding the Data passed back, the above cases don't really need to know its type, but in the case of request-specific interceptors that could be useful. Do you have some cases in mind where this would be beneficial?

Makes sense; let's not overcomplicate it. If users really want to, I suppose they could have their interceptor manually decode the proto data, modify it, then serialize it again. That way, we could have an interface like this (I think):

protocol Interceptor {
init?()  // when returning nil, the request should not be handled by this interceptor? (just an idea)
func onStart(method: String, metadata: Metadata, completion: (String, Metadata) -> Void) // alternatively, we could move `onStart` into init — might make more sense actually.
func onRequest(request: Data, completion: (Data) -> Void) // called for each request message, gives us the option to manipulate any data sent by the client before it is passed to the network
func onResponse(response: Data, completion: (Data) -> Void) // called for each response message, gives us the option to manipulate any data sent by the server before it is passed to the client
func onStatus(status: ServerStatus, completion: (ServerStatus) -> Void) // called once a status has been received
}

extension Interceptor {
func onStart(method: String, metadata: Metadata, completion: (String, Metadata) -> Void) { completion(method, metadata) }
// same for the other `on...` methods
}

We could even split that out into different interceptor types for request creation, sending, receiving, and status
Could you add some examples of how this would be used? Trying to visualize it a bit better.

Scratch that, it would just overcomplicate things ;-)

[rebello95]

Sorry for the delayed response 😄looks like we're on the same page, though! I went ahead and updated the proposal document and split it into 2 implementation options (sync and async). Please take a look when you get a chance - the async approach is very similar to what we discussed above, with a few additional tweaks. Sounds like that's the approach both of us prefer at the moment.

One note on the onStart function you described above - I opted to keep init and onStart separate in the proposal so that interceptors have the ability to defer requests if they need to from the onStart function (it felt a bit weird to add a completion closure to an optional init).

[MrMage]

Looks good; I prefer the async approach (mostly looked at that one). A few remarks:

• a few @escaping are missing.
• should we allow interceptors to cancel a request via the onStart etc. callbacks, e.g. by adding another argument to the completion handler (or using "cancel/Metadata" or "cancel/Data" enums for the argument)?
• nit: I would change "Called for every request" to "Called for every request message sent" to make it clearer that this is not about gRPC calls, but individual proto messages.

[rebello95]

Good callouts, updated accordingly. Regarding the second point about canceling a request, I went with case cancel, case next(Metadata). Do you think we should support canceling requests from onRequest as well, or just onStart? My thinking is that onStart should take care of it since it's unlikely a stream would need to be canceled after it's started.

[MrMage]

Do you think we should support canceling requests from onRequest as well, or just onStart? My thinking is that onStart should take care of it since it's unlikely a stream would need to be canceled after it's started.

SGTM, we can still add it later on :-)

[rmahindra]

@MrMage Is the interceptor or middleware functionality added in grpc for ios?

[MrMage]

@MrMage Is the interceptor or middleware functionality added in grpc for ios?

No.

[MrMage]

The CgRPC implementation is now officially deprecated; closing this for now — we would probably want a new proposal for the nio branch.