Refactor the async server call context #1407
Conversation
Motivation: The async call context provided to async server methods provides an API to access request headers and the logger as well as modify the response headers, trailers and user info. This is currently implemented as a class with its mutable state protected by a lock. This works fine but is racy: the response headers can be updated after writing the first message and still be sent before that message (as writing the message requires executing onto the event loop). Modifications: - Turn the context into a `struct` and break it into request and response components. - Request components are immutable (or, more correctly, mutating them has no impact on the underlying RPC). - Response components are mutable via `async` calls which execute onto the underlying event loop. - This introduces allocations as the work is submitted onto the event loop, however setting headers is not a frequent operation and is worth the trade off for a less surprising API. - Trap when headers/trailers are set after they have been sent (this is an assertion failre). Result: - Async server context has a less surprising API - Setting headers/trailers is not racy
| func testResponseHeadersDroppedIfSetAfterFirstResponse() async throws { | ||
| throw XCTSkip("Setting metadata is racy. This test will not reliably pass until that is fixed.") |
There was a problem hiding this comment.
This was removed because it no longer applies: we now trap (on debug builds) if headers are set after writing the first response
| /// The request headers received from the client at the start of the RPC. | ||
| public var headers: HPACKHeaders | ||
|
|
||
| /// A logger. | ||
| public var logger: Logger | ||
|
|
There was a problem hiding this comment.
Should we expose the setters publicly here as well?
There was a problem hiding this comment.
There's no harm in having them: the caller would have to take a copy of Request to mutate these anyway.
| public func setHeaders(_ headers: HPACKHeaders) async throws { | ||
| try await self.contextProvider.setResponseHeaders(headers) | ||
| } | ||
|
|
||
| /// Set the metadata to return at the end of the RPC. | ||
| /// | ||
| /// If this is required it must be updated before returning from the handler. | ||
| public func setTrailers(_ trailers: HPACKHeaders) async throws { | ||
| try await self.contextProvider.setResponseTrailers(trailers) | ||
| } |
There was a problem hiding this comment.
The new API only allows to set headers/trailers. Should we expose APIs that allow to mutate the currently set headers?
There was a problem hiding this comment.
Good question. I thought about this a bit and I don't think we should provide it.
They are empty by default and the caller knows what the value is: it can only have been set by them so I'm not sure it's actually useful to have (any update could be achieved by just setting the value a bit later).
Motivation:
The async call context provided to async server methods provides an API
to access request headers and the logger as well as modify the response
headers, trailers and user info.
This is currently implemented as a class with its mutable state
protected by a lock. This works fine but is racy: the response headers
can be updated after writing the first message and still be sent before
that message (as writing the message requires executing onto the event
loop).
Modifications:
structand break it into request andresponse components.
asynccalls which execute ontothe underlying event loop.
event loop, however setting headers is not a frequent operation and is
worth the trade off for a less surprising API.
(this is an assertion failre).
Result: