Add connection lifecycle handling - #92
Conversation
# Conflicts: # Sources/NIOHTTPServer/NIOHTTPServer+HTTP1_1.swift
d879cfb to
413211a
Compare
| let connectionLogger: Logger = { | ||
| var logger = rootLogger | ||
| let peer = context.remoteAddress.map { "\($0)" } ?? "unknown" | ||
| logger[metadataKey: "peer"] = .string(peer) | ||
| logger[metadataKey: "http"] = .string("\(context.httpVersion)") | ||
| return logger | ||
| }() | ||
| connectionLogger.info("connection accepted") | ||
| defer { connectionLogger.info("connection closed") } |
There was a problem hiding this comment.
Let's use the new task local Logging APIs for this example. Actually the HTTP Server itself should set the task local logger and configure it correctly with metadata. This shouldn't be up to the user.
There was a problem hiding this comment.
what metadata should be included? In some applications it may not be desirable to log certain things like peer IP.
There was a problem hiding this comment.
The HTTP server should make a decision about the general useful metadata it wants to include. I would say things like HTTP version and peer IP are sensible. The rest the user can extract from the context and add.
There was a problem hiding this comment.
IMO, peer IP is sensitive and shouldn't be logged by default
There was a problem hiding this comment.
Can we keep the docs updates like this separate?
There was a problem hiding this comment.
Even when it's related to the change being introduced in the PR?
| extension NIOHTTPServer { | ||
| /// Connection-specific information available during request handling. | ||
| /// The application-level HTTP version negotiated for a connection. | ||
| public enum HTTPVersion: Sendable, Hashable { |
There was a problem hiding this comment.
This should also become part of HTTPAPIs
There was a problem hiding this comment.
gjcairo
changed the title
ehaydenr
left a comment
ehaydenr
left a comment
There was a problem hiding this comment.
This is looking good at a high level. Can we split it up into two PRs, though? I think CloseableConnection might make sense to have as a subsequent PR.
gjcairo
added
🆕 semver/minor
| } | ||
| } catch { | ||
| self.logger.debug( | ||
| "Error tearing down HTTP/1.1 channel", |
There was a problem hiding this comment.
executeThenClose doesn't throw if there was an error closing the channel. It throws the error that was caught in the body, so the "tearing down" part of the log statement isn't accurate here.
| self.logger.debug( | ||
| "Error thrown by connection handler", | ||
| metadata: ["error": "\(error)"] | ||
| ) |
There was a problem hiding this comment.
We should add a try? await connectionChannel.close() here.
Otherwise, the connection does not get cleaned up if an error is thrown before handleRequests is called.
There was a problem hiding this comment.
Edit: try? await connectionChannel.close() should be after the do-catch block so that we also clean up the connection if handleConnection did not call handleRequests.
| /// derived. | ||
| public protocol PeerCertificate: RequestContext { | ||
| /// The peer's validated certificate chain, when available. | ||
| var peerCertificateChain: X509.ValidatedCertificateChain? { get async throws } |
There was a problem hiding this comment.
Why is this async and throws? That seems very odd
There was a problem hiding this comment.
This was already its signature - I'm just introducing this capability. Look at NIOHTTPServer+ConnectionContext.swift:ConnectionContext/peerCertificateChain.
Based on how we're getting it, it has to be async throws.
There was a problem hiding this comment.
That is highly unfortunate since it should be synchronously present. Let's file an issue to track this since in the HTTPAPIs we should not make the property async or throws
There was a problem hiding this comment.
Why should it be synchronously present and not throw?
This is currently async because we're are calling get() on an ELF. We could just block if we need it to be sync.
We can't get around the throws unless we try?, but is hiding the errors desired?
There was a problem hiding this comment.
Because at the point where we hand the connection to the user we already have the TLS connection established. So we should be able to get the peer cert before we hand the connection to the user
There was a problem hiding this comment.
Sure. We can probably get it via sync operations. I'll create a follow up issue.
5 participants
Summary
Add a connection-lifecycle surface to
NIOHTTPServerso users can do per-connection setup (such as loggers, metrics, atomic counters, etc) and share state across requests on the same connection. The existingserve(handler:)entry point is preserved unchanged at the call site.What's new
NIOHTTPServer.serve(connectionHandler:): the connection-aware counterpart toserve(handler:).NIOHTTPServerConnectionHandlerprotocol withhandleConnection(connection:context:).NIOHTTPServer.Connection: struct that represents an active HTTP server connection. Consumed by exactly one call tohandleRequests; if the handler returns without callinghandleRequests, the connection is dropped cleanly on scope exit.NIOHTTPServer.ConnectionContext, which now exposeshttpVersion,remoteAddress,localAddress, the existingpeerCertificateChain, and a newsignalConnectionClose().HTTPServerCapabilitycapabilities:ConnectionInfo,PeerCertificate.NIOHTTPServer.RequestContextis now a concrete struct conforming them, so request handlers reach connection-scoped data viarequestContext.remoteAddress, etc.@TaskLocal NIOHTTPServer.connectionContextAPI is removed, and all access is now throughrequestContext.