Add connection lifecycle handling#92
Conversation
# Conflicts: # Sources/NIOHTTPServer/NIOHTTPServer+HTTP1_1.swift
gjcairo
force-pushed
the
worktree-connection-lifecycle
branch
from
d879cfb to
413211a
Compare
| /// peer's address and the local address the connection is bound to. Both | ||
| /// are reported best-effort: implementations may return `nil` when the | ||
| /// underlying transport cannot report an address. | ||
| public protocol ConnectionInfo: RequestContext { |
There was a problem hiding this comment.
I think we really wanna add that to the HTTPAPIs. Can you file an issue there?
There was a problem hiding this comment.
What would be used as the SocketAddress type in HTTPAPIs?
There was a problem hiding this comment.
We need to add a new SocketAddress currency type in the HTTPAPIs that we need to land in the planned network currency types package
There was a problem hiding this comment.
| /// peer's mTLS-validated certificate chain (when applicable). Implementations | ||
| /// return `nil` if mTLS isn't configured, or if no validated chain was | ||
| /// derived. | ||
| public protocol PeerCertificate: RequestContext { |
There was a problem hiding this comment.
| 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.
3 participants
Summary
Add a connection-lifecycle surface to
NIOHTTPServerso users can do per-connection setup (such as loggers, metrics, atomic counters, etc), share state across requests on the same connection, and signal that the current connection should close after the in-flight response. 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,CloseableConnection.NIOHTTPServer.RequestContextis now a concrete struct conforming to all three, so request handlers reach connection-scoped data viarequestContext.remoteAddress,requestContext.signalConnectionClose(), etc.@TaskLocal NIOHTTPServer.connectionContextAPI is removed, and all access is now throughrequestContext.