Add debug channel callbacks #52
Conversation
Motivation: Sometimes it can be useful to modify the channel pipeline or inspect the channel when debugging. v1 has hooks to allow users to do this which proved helpful a number of times. v2 should offer similar behaviour. Modifications: - Add 'DebugChannelCallbacks' to the client and server config and wire them up appropriately. - Add tests. Extract 'TransportKind' from some tests as it was being recreated a number of times. Result: Users can modify the channel pipeline if they need to.
|
API breakages are expected; we added new parameters to these: |
| public var onCreateTCPConnection: (@Sendable (_ channel: any Channel) async throws -> Void)? | ||
|
|
||
| /// A callback invoked with each new HTTP/2 stream. | ||
| public var onCreateHTTP2Stream: (@Sendable (_ channel: any Channel) async throws -> Void)? |
There was a problem hiding this comment.
Are these actually async throws? They're neither in the init below, so one of the two appears to be wrong.
Same for the server callbacks.
There was a problem hiding this comment.
Oof. Not sure what happened there! I can only assume the autogenerated memberwise initialiser didn't work as expected. These APIs should line up.
There was a problem hiding this comment.
Regarding whether they should be async throws vs. EventLoopFuture<Void>, I flip-flopped a bit on this. I think async throws is more appropriate: this API doesn't have to be used for configuring a channel pipeline; it's just that most uses will configure the pipeline.
There was a problem hiding this comment.
I thought about this some more and think I've changed my position on this. We're slotting directly into NIO land here so exposing the natural NIO API makes more sense and I think users of this API are more likely to configure the channel pipeline than anything else. So yeah, I think you're right that we should make this EventLoopFuture<Void>.
| self.whenComplete { result in | ||
| switch result { | ||
| case .success(let value): | ||
| Task { |
There was a problem hiding this comment.
I understand this should be used for debugging only, so maybe the fact we're spawning a Task here isn't so bad. But I wonder if we can avoid this anyways - one downside is this for example won't respect task executor preferences, and that may cause some unintended consequences even when debugging.
So I guess this is related to my comment above: do we actually want these callbacks to be async?
There was a problem hiding this comment.
... this for example won't respect task executor preferences,
Is that actually true? (I don't know either way.)
If so the workaround is straightforward: call withTaskExecutorPreference within the callback.
There was a problem hiding this comment.
It is true as far as I understand. The proposal states that unstructured tasks (using both Task {} and Task.detached {}) do not inherit task executor preference. There's a separate Task(executorPreference: someExecutor) {} constructor to do this.
You could call withTaskExecutorPreference, but unless there is some magic behind the scenes, this will just cause an extra isolation domain hop, since we'll have to hop to the main executor first if we're not there already to execute the Task's closure, and then hop to the preferred executor isolation domain to execute whatever's inside the callback.
Tests/GRPCNIOTransportHTTP2Tests/HTTP2ServerTransport+DebugTests.swift
Outdated
Show resolved
Hide resolved
…ts.swift Co-authored-by: Gus Cairo <[email protected]>
Motivation:
Sometimes it can be useful to modify the channel pipeline or inspect the channel when debugging. v1 has hooks to allow users to do this which proved helpful a number of times. v2 should offer similar behaviour.
Modifications:
Result:
Users can modify the channel pipeline if they need to.