Add "debug initializer" hook for channels #801
Conversation
Motivation: As requested in swift-server#596, it can be handy to have a lower-level access to channels (HTTP/1 connection, HTTP/2 connection, or HTTP/2 stream) to enable a more fine-grained interaction for, say, observability, testing, etc. Modifications: - Add 3 new properties (`http1_1ConnectionDebugInitializer`, `http2ConnectionDebugInitializer` and `http2StreamChannelDebugInitializer`) to `HTTPClient.Configuration` with access to the respective channels. These properties are of `Optional` type `@Sendable (Channel) -> EventLoopFuture<Void>` and are called when creating a connection/stream. Result: Provides APIs for a lower-level access to channels.
| http1_1ConnectionDebugInitializer: | ||
| (@Sendable (Channel) -> EventLoopFuture<Void>)? = nil, | ||
| http2ConnectionDebugInitializer: | ||
| (@Sendable (Channel) -> EventLoopFuture<Void>)? = nil, | ||
| http2StreamChannelDebugInitializer: | ||
| (@Sendable (Channel) -> EventLoopFuture<Void>)? = nil |
There was a problem hiding this comment.
Adding parameters to a function is a SemVer breaking change, even if they have defaults.
The reason is that you could store the function in a variable:
struct Foo {
var bar: Int
init(bar: Int) { ... }
}
// This is totally value
let makeFoo = Foo.init(bar:)
let foo = makeFoo(bar: 42) // equivalent to Foo(bar: 42)If you subsequently add a parameter to the init then this is an API break because init(bar:) won't exist, even if there's a default for the param you add.
The upshot of this is that you should add a new init rather than updating the existing ones.
| private func executeRequest0( | ||
| _ request: HTTPExecutableRequest, | ||
| streamChannelDebugInitializer: (@Sendable (Channel) -> EventLoopFuture<Void>)? | ||
| ) { |
There was a problem hiding this comment.
I don't think this API should change. Instead I think the initialiser should be passed in to the start method. The initialiser isn't something that changes; it remains the same for all requests so it's really a property of the connection.
| } | ||
| } | ||
|
|
||
| class DebugInitializerUtil { |
There was a problem hiding this comment.
This isn't a very helpful name, something like CountingDebugInitializer would be more appropriate as it hints to what it actually does.
| var executionCount: Int | ||
|
|
||
| @Sendable | ||
| func operation(channel: Channel) -> EventLoopFuture<Void> { |
There was a problem hiding this comment.
in light of the comment above re: naming, initialize would be a more obvious function name.
| } | ||
|
|
||
| final class CountingDebugInitializerUtil: Sendable { | ||
| let executionCount: NIOLockedValueBox<Int> |
There was a problem hiding this comment.
A useful thing to do is to make this private and have a computed property which returns the value from the box; this makes it easier for any test which wants the value to get it without having to go via withLockedValue.
| let executionCount: NIOLockedValueBox<Int> | ||
|
|
||
| /// The acual debug initializer. | ||
| @Sendable |
There was a problem hiding this comment.
I don't think we need this annotation as the type is Sendable
There was a problem hiding this comment.
A warning is given for the implicit conversion.
There was a problem hiding this comment.
I think that's because you do http1_1ConnectionDebugInitializer: connectionDebugInitializerUtil.initialize(channel:) rather than:
http1_1ConnectionDebugInitializer: { channel in
connectionDebugInitializerUtil.initialize(channel: channel)
}| ) | ||
| defer { XCTAssertNoThrow(client.shutdown()) } | ||
|
|
||
| let bin = HTTPBin(.http1_1(ssl: true, compress: false)) |
There was a problem hiding this comment.
Presumably the initialiser also works when we're not using SSL? I'm not super familiar with this codebase so would be good to check that.
| func initialize(channel: Channel) -> EventLoopFuture<Void> { | ||
| self._executionCount.withLockedValue { $0 += 1 } | ||
|
|
||
| return channel.eventLoop.makeSucceededVoidFuture() |
There was a problem hiding this comment.
It may be useful to add some tests that have the debug initializer actually delay the setup. That is, make sure that we actually do wait for this to complete. Easiest thing to do might be to set some low timeouts, and then confirm the timeouts fire, and that completing the promise doesn't then cause any issues.
Co-authored-by: Cory Benfield <[email protected]>
|
Thanks @clintonpi -- this'll be a really handy feature! |
Motivation:
As requested in #596, it can be handy to have a lower-level access to channels (HTTP/1 connection, HTTP/2 connection, or HTTP/2 stream) to enable a more fine-grained interaction for, say, observability, testing, etc.
Modifications:
http1_1ConnectionDebugInitializer,http2ConnectionDebugInitializerandhttp2StreamChannelDebugInitializer) toHTTPClient.Configurationwith access to the respective channels. These properties are ofOptionaltype@Sendable (Channel) -> EventLoopFuture<Void>and are called when creating a connection/stream.Result:
Provides APIs for a lower-level access to channels.