Relax Sendable requirements for interceptors
#182
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@eseay would you mind taking a look at this? 🙏🏽 |
eseay
approved these changes
Sep 25, 2023
rebello95
added a commit
that referenced
this pull request
Oct 16, 2023
## Summary This PR refactors the interceptor implementation offered by Connect-Swift to support **asynchronous processing inside interceptors**. This is useful when clients want to perform work prior to sending outbound requests or returning responses to callers, such as: - Refreshing an OAuth token before attaching it to the outbound request - Rejecting an outbound request (and preventing it from being sent) based on the result of some asynchronous work - Waiting to make decisions on requests/responses until more data has arrived (such as waiting to do something with a stream before request data has been passed to an interceptor) Resolves #104. Docs are being updated in connectrpc/connectrpc.com#68. ## Considerations When implementing these, the following considerations were made: - Allowing interceptors to fail/reject outbound requests ([like Alamofire does](https://github.com/Alamofire/Alamofire/blob/master/Documentation/AdvancedUsage.md#adapting-and-retrying-requests-with-requestinterceptor)) - This PR adds this functionality - Whether we should expose async/await interfaces or callbacks/closures - In practice, exposing async/await interfaces wouldn't dramatically change interceptor implementations (it'd effectively be `return await finishProcessingRequest(request)` versus `proceed(request)`), but it would place significant limitations on interceptors by disallowing them from doing any buffering on streams due to the fact that data cannot be passed to any interceptors while one is being `await`ed. For this reason, we opted for exposing closures which allows interceptors to `proceed()` the chain at any point, potentially after receiving additional invocations from the client with more information. Implementations can still wrap these calls with async/await if desired (example below, and in `Interceptor.swift`) - Whether we should enforce additional guardrails for streaming to ensure an interceptor continues interceptor iteration in the right order, such as `headers -> data -> data -> data -> trailers` Example of wrapping an interceptor with async/await: ```swift final class AsyncInterceptor: Interceptor, Sendable { func unaryFunction() -> UnaryFunction { return .init { request, proceed in Task { proceed(await self.handleRequest(request)) } } responseFunction: { response, proceed in Task { proceed(await self.handleUnaryResponse(response)) } } responseMetricsFunction: { metrics, proceed in Task { proceed(await self.handleUnaryResponseMetrics(metrics)) } } } func streamFunction() -> StreamFunction {...} } ``` ## Approach - Interceptors are expected to be instantiated **once per request or stream**. - Each interceptor has the opportunity to perform asynchronous work before passing a potentially altered value to the next interceptor in the chain. When the end of the chain is reached, the final value is passed to the networking client where it is sent to the server or to the calling client. - Interceptors may also fail outbound requests before they're sent, thus preventing subsequent interceptors from being invoked and returning a specified error back to the original caller. - Interceptors are closure-based and are passed both the current value and a closure which should be called to resume the interceptor chain. Propagation will not continue until this closure is called. Additional values may still be passed to a given interceptor even though it has not yet continued the chain. - Implementations should be thread-safe (hence the `Sendable` requirement on interceptor closures), as closures can be invoked from different threads during the span of a request or stream due to the asynchronous nature of other interceptors which may be present in the chain. - Interceptors can also be written using async/await by incorporating a `Task` (as shown above). ## Notes - New tests cover manually invoking the interceptor chain as well as sending and receiving data through the chain with a real client - Some of the changes from #182 have been reverted as part of this PR due to the fact that interceptor closures must be `Sendable`/thread-safe per the discussion above - I built this branch locally with `swiftSettings: [.unsafeFlags(["-Xfrontend", "-strict-concurrency=complete"])]` to ensure that it doesn't introduce new strict concurrency warnings - This PR also replaces some "filter" wording with "interceptor" for consistency (I've spent too much time in Envoy land in the past...)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instances of interceptors are not expected to be called concurrently from multiple threads, and the
Sendablerequirements currently imposed on them make it challenging for consumers to store state on the interceptors. This PR relaxes theSendablerequirements for interceptors, making it easier for consumers to store and access properties on them.Relatedly, it looks like there may be a Swift bug which prevents annotating properties with
@MainActorfrom properly handling sendability: https://forums.swift.org/t/is-this-sendable-warning-correct/55632/2This PR builds without warnings on Xcode 15 with strict concurrency rules enabled.