Reset streams when throwing from request handler - #115
Conversation
| extension NIOHTTPServer { | ||
| /// The application-level HTTP version negotiated for a connection. | ||
| public enum HTTPVersion: String, Sendable, Hashable { | ||
| public enum HTTPVersion: String, Sendable, Hashable, CaseIterable { |
There was a problem hiding this comment.
We need to mark this as @nonexhaustive otherwise we won't be able to support a hypothethical HTTP/4.
| // A throwing handler signals that the exchange failed. The error is deliberately not propagated to any | ||
| // caller: it exists to drive the wire, aborting the exchange with protocol error codes the error can choose | ||
| // by conforming to `NIOHTTPServerHTTP2StreamResetError` / `NIOHTTPServerHTTP3StreamResetError`. | ||
| self.logger.error( |
There was a problem hiding this comment.
This must not log at error level. Libraries should only log at info level and below. I think this is still somewhat the normal case so I would recommend logging at trace or debug level.
There was a problem hiding this comment.
I've changed this and also updated other logs where we were using error
| self.logger.error( | ||
| "Error thrown while handling request: aborting.", | ||
| metadata: [ | ||
| "error": "\(error)", |
There was a problem hiding this comment.
swift-log has new APIs that you can pass an error to. Can we use that instead?
There was a problem hiding this comment.
I updated other instances to use the new API too
| "Error thrown while handling request: aborting.", | ||
| metadata: [ | ||
| "error": "\(error)", | ||
| "protocol": "\(requestContext.connectionContext.httpVersion)", |
There was a problem hiding this comment.
I would recommend that we move all the keys into a internal struct called LoggingKeys so we don't have random string literals floating around
| /// // ... | ||
| /// } | ||
| /// ``` | ||
| public protocol NIOHTTPServerHTTP2StreamResetError: Error { |
There was a problem hiding this comment.
I would recommend a different name here following what GRPC does https://github.com/grpc/grpc-swift-2/blob/76daaeaf345213fbd1b0ae78121c06036a0ddbfe/Sources/GRPCCore/RPCError.swift#L309
| public protocol NIOHTTPServerHTTP2StreamResetError: Error { | |
| public protocol NIOHTTPServerHTTP2StreamResetErrorConvertible: Error { |
| /// // ... | ||
| /// } | ||
| /// ``` | ||
| public protocol NIOHTTPServerHTTP3StreamResetError: Error { |
There was a problem hiding this comment.
Same here
| public protocol NIOHTTPServerHTTP3StreamResetError: Error { | |
| public protocol NIOHTTPServerHTTP3StreamResetErrorConvertible: Error { |
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| public import NIOHTTP2 |
There was a problem hiding this comment.
I'm not happy about this public import. I would prefer if we can continue to avoid exporting NIO into our public API where possible. We should find a better place for this error to live. Maybe even this package
There was a problem hiding this comment.
Fair. I changed the type here to UInt32 to remove the NIO dependency and to leave it up to implementers to decide where they get the errors codes from (I've added an example too showing you can get the errors from the NIO H2 error codes' raw value). Let me know what you think. Like this, we could upstream these protocols to the API proposal package eventually.
(I know we generally expose Int in APIs, but only valid values for H2 steam error codes are positive integers up to 32 bits so it seems better to enforce it like this).
| //===----------------------------------------------------------------------===// | ||
|
|
||
| #if HTTP3 | ||
| public import HTTP3 |
There was a problem hiding this comment.
I did the same here, but using UInt64, for the same reasons as I explained above.
| metadata: [ | ||
| "failure-reason": .string(error.reason) | ||
| ] | ||
| metadata: [LoggingKeys.failureReason: .string(error.reason)] |
There was a problem hiding this comment.
Why don't we pass the error to the log here. Then we can remove that metadata completely
| /// `CONNECT_ERROR`, or `0x02` for `INTERNAL_ERROR`. | ||
| /// | ||
| /// This code is used only when the request is served over HTTP/2. | ||
| var http2StreamResetCode: UInt32 { get } |
There was a problem hiding this comment.
Did you consider using https://github.com/apple/swift-nio-http2/blob/281170341aeff301a62dab0ba13f9829ff148989/Sources/NIOHTTP2/HTTP2ErrorCode.swift#L21 instead of a raw UInt32?
4 participants
This change translates errors thrown from the request handler into stream resets.
Two new protocols have been added:
NIOHTTPServerHTTP2StreamResetErrorandNIOHTTPServerHTTP3StreamResetError, which allow to specify how custom errors map to HTTP/2 stream reset error codes and and HTTP/3 stream reset and stop sending error codes, respectively.The behaviour of throwing an error changes depending on which protocol the request is being served over:
Connection: close.RST_STREAMis sent over the wire, closing the stream bidirectionally. If the error thrown conforms toNIOHTTPServerHTTP2StreamResetError, the mapped error code will be used; if not, the code will be defaulted toINTERNAL_ERROR(0x02).STREAM_RESETandSTOP_SENDINGare both sent to the client, closing the stream bidirectionally. If the error thrown conforms toNIOHTTPServerHTTP3StreamResetError, the mapped error code will be used; if not, the code will be defaulted toH3_INTERNAL_ERROR(0x0102).