-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support h2 stream resets through user events #241
base: main
Are you sure you want to change the base?
Conversation
Resetting streams with specific error codes is required by some applications such as those implementing the CONNECT method (see https://datatracker.ietf.org/doc/html/rfc9113#section-8.5-8). Unfortunately, the HTTP2ToHTTP codecs don't expose this capability to applications. This change introduces an outbound user event applications can trigger when needing to reset a stream.
} | ||
|
||
/// Events that can be sent by the application to be handled by the `HTTP2StreamChannel` | ||
public struct HTTP2FramePayloadToHTTPEvent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need a NIO
prefix as it's new API (see https://github.com/apple/swift-nio/blob/main/docs/public-api.md#1-no-global-namespace-additions-that-arent-prefixed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should also be Hashable
and Sendable
, it's generally useful to add these to value types.
} | ||
|
||
/// Returns reset code if the event is a reset | ||
public func reset() -> HTTP2ErrorCode? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A computed property would be more idiomatic here
if case let .reset(code) = self.kind { | ||
return code | ||
} | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: convention is NIO is to exhaustively switch over enums
fileprivate enum Kind { | ||
case reset(HTTP2ErrorCode) | ||
} | ||
|
||
fileprivate var kind: Kind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make these private and use the public API in the handlers above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks Eric!
Allow applications to trigger HTTP/2 stream resets while using NIOHTTPTypesHTTP2's codecs
Motivation:
Resetting streams with specific error codes is required by some applications such as those implementing the CONNECT method
(see https://datatracker.ietf.org/doc/html/rfc9113#section-8.5-8). Unfortunately, the HTTP2ToHTTP codecs don't expose this capability to applications.
Modifications:
Introduce an outbound user event applications can trigger when needing to reset an HTTP/2 stream.
Result:
Now applications can trigger HTTP/2 stream resets while using the codecs provided by NIOHTTPTypesHTTP2