H3 upgrade (Alt-Svc) - #18
Merged
Merged
Conversation
A TLS client that negotiates no ALPN at all -- curl --no-alpn, or anything that offers only protocols the server does not know -- left `session` unset after the handshake, so the connection was dropped without an answer. Refusing those buys nothing: HTTP/1.1 is what a connection without a negotiated protocol speaks anyway. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
HTTP/3 is not something a TCP connection can turn into: there is no `Connection: Upgrade` on the way to QUIC. What there is instead is RFC 7838, where the server says where else it can be reached and the client makes its *next* connection there. The server puts its own HTTP/3 endpoint into every response it sends over HTTP/1.1 and HTTP/2, but never over HTTP/3, which is already there. As QUIC shares the endpoint the TCP acceptor is listening on, the alt-authority is a port and nothing else. `server::Config::alt_svc_max_age` is how long a client may remember it, and 0s advertises nothing at all. The client acts on it only with `client::Config::follow_alt_svc` set, and then it takes precedence over `Config::protocol`. It reads the header field from HTTP/1.1 and HTTP/2 responses and, over HTTP/2, the ALTSVC frame as well, which lets a server advertise before the first request has been sent. The alternative is kept for as long as `ma` says, but only in memory and only for the lifetime of the Client. The origin does not change with any of this, only where it is reached. The parser is shared by both sources and skips alternatives it does not understand instead of failing the whole field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A request with "Connection: close", and any HTTP/1.0 request that does not ask for a persistent connection, was served and the connection was closed right after -- but the response never said so. A client reading it sees a plain "HTTP/1.1 200" and expects the connection to stay up, with no way of telling the close that follows from a connection lost mid-message (RFC 9112, section 9.6). The response keeps its own version: answering 1.0 would rule out the chunked framing used for responses without a content length, and an explicit "Connection: close" is unambiguous either way. The tests drive a raw socket, so they see what goes over the wire and whether the connection ends or is dropped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The server closed the socket and only then tried to shut it down, which always failed with EBADF -- the FIXME on it was right. Closing a socket that still has unread data in its receive queue answers the peer with an RST, and an RST discards what has not been delivered yet: the peer can lose the very response that told it the connection was ending. So shut the sending side down first, and let go of the socket after that. A client writing into the teardown now sees EPIPE where it used to see ECONNRESET, depending on which write the reset catches, so the test for it accepts either. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The sessions ended a TLS connection by shutting down the TCP socket beneath it, which is a truncated stream as far as TLS is concerned: the peer can not tell the end of the data from a connection that was cut, and every read after the last response fails with ssl::error::stream_truncated instead of a clean end of stream. OpenSSL calls it "unexpected eof while reading". Ending the stream itself is asynchronous -- "close_notify" goes out and the peer answers with one of its own -- so it is an awaitable, called from the session coroutines before the FIN. A peer that never answers must not keep the session around for good, so the wait for it is bounded: the connection is going away either way. Streams that have nothing of their own to end complete right away. The type-erased stream forwards to whatever it wraps, which is what the unused async_shutdown_impl() hook was waiting for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Set BeforeLambdaBody to false and LambdaBodyIndentation to Signature, then reformatted all tracked sources. Asio code is dense with short lambdas, and giving each one a brace line of its own plus outer-scope indentation cost a line per lambda while visually detaching the body from its capture list. Whitespace only: the code is token-identical to the previous commit, apart from reflowed comments, a sorted include pair in request_handlers.hpp and a namespace closing comment in test_formatter.cpp. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…handles
Each of the four public interfaces -- {server,client}::{Request,Response} --
wraps exactly one of the two abstract bases the protocol backends implement,
but that split never reached the public layer. Anything interested in only the
reading or only the writing half of a message, drain() above all, had to be a
template to accept both sides.
So mirror the implementation hierarchy in the handles. Reader and Writer are
handle classes of their own now, and the four interfaces derive from one each
and add no data members, recovering their own Impl through a static_cast in
pimpl(): there is still exactly one shared_ptr per handle, and moving one into
a bare Reader or Writer hands off that half of the message with its ownership
intact. The destructors stay non-virtual, as these are never owned through a
base pointer.
drain(), read(), try_receive() and send_eof() are plain functions over Reader&
and Writer& now, and send(), sendAndForceEOF() and generate() no longer take
the writer as a template parameter. No call site changes.
The abstract bases move along, out of the anyhttp::impl namespace -- which is
gone -- and into reader.hpp and writer.hpp as Reader::Impl and Writer::Impl,
which is how every other Impl in the library is spelled. The four derived Impl
classes keep their names and lose the ReaderOrWriter alias, which never had a
user.
Merging the four copies of the Asio boilerplate forced a decision in two
places where the two sides had drifted apart:
server::Response bound get_executor() to its write initiations, while
client::Request bound whatever the token brought, because the implementation
may already be gone by the time a write is started (the SpawnAndForget test).
Writer binds get_associated_executor(token, get_executor()), which is what the
server already got -- a token's own executor takes precedence either way --
and get_executor() answers with an empty executor instead of asserting once
the handle has been released. Such a write fails with bad_descriptor without
touching an executor at all.
The buffer sequence overload of async_read_some() fell off the end of a
non-void function when every buffer in the sequence was empty. It forwards an
empty buffer now, which Reader::Impl already defines as an immediate,
zero-byte success.
Incidentally, server::Request gains that sequence overload and client::Response
gains content_length(), both of which only one side used to have.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A request has a method and a URL, a response has a status code -- but both Reader implementations declared all three, with a FIXME admitting that a status code on server::Request::Impl makes no sense. The mirror image was url() on client::Response::Impl, which handed out the URL of the *request* and was never called. Both are gone now; fields() stays on both, being the one part of a message header that really is symmetric. server::Request gains a public method(), which had no way of reaching the request handler before. All three backends already keep the method on the stream, so nothing new has to be parsed or stored for it. What this cost is the single reader template per backend. NGHttp2Reader, Http3Reader and BeastReader are each instantiated once per role and marked the role-specific accessors "override", which stops working the moment the two interfaces stop declaring the same set. Constraining a member is not an option -- a virtual function may not have a trailing requires-clause -- and dropping "override" would only move the meaningless member from the interface into the backend. So each reader now splits into the shared part and a thin role class next to where it is built, leaving the backend headers free of server_impl.hpp/client_impl.hpp. That split pays for itself in h1: the role class fixes the parser type, so status_code() loses its "if constexpr (Parser::is_request())" branch, m_url moves down into the request reader where it belongs, the dead m_status_code member is gone, and both call sites get considerably shorter. The dump handler now reports the method, which puts ClientAsync.Dump behind it on all three protocols; the h2c upgrade test checks for it as well, that path carrying the method across the handover to HTTP/2 separately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…er and client tests
The inline mermaid diagram had drifted out of sync with the actual class hierarchy and was awkward to extend. Render it from a drawio SVG instead, shown in a new "Class Hierarchy" section. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Neither is referenced any more now that the overview diagram lives in docs/. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The file still carried the Microsoft boilerplate from the template this repository started out as. Attribute it properly and drop the stray indentation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The backend interfaces lived inside reader.hpp and writer.hpp, so every user of the public handles -- client.hpp and server.hpp, and with them anything that merely wants to read or write a body -- pulled in the full virtual interface a protocol backend has to implement. Split them out into reader_impl.hpp and writer_impl.hpp, matching the client_impl.hpp / server_impl.hpp / session_impl.hpp convention, and leave a forward declaration behind. The handles need no more than that: their destructor, move constructor and move assignment were already out-of-line, which is what keeps shared_ptr<Impl> happy against an incomplete type. Only code that implements or narrows to an Impl now includes the new headers: client_impl.hpp and server_impl.hpp derive from them, and h2_stream.hpp and h3_stream.hpp keep Reader::Impl* / Writer::Impl* back pointers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Seven headers pulled in all of Boost.Asio -- client_impl.hpp, server_impl.hpp, h1_session.hpp, h2_session.hpp, h2_stream.hpp and the two detection helpers under detail/ -- and with them every backend translation unit that includes any of those. Replace each with the handful of headers the file actually needs, and do the same for the sources and the test fixture header. The global "using namespace boost::asio;" in h1_session.hpp and h2_session.hpp is what let this go unnoticed: names like co_spawn, detached, make_strand and ip::v6_only were reaching their users through the umbrella, so those users now include what they use. sender.cpp under src/research is left alone; no target builds it. This trims about 9% off the preprocessed size of a typical backend translation unit (~30k lines of roughly 330k). It does not show up in clean-rebuild wall time, which on this machine stays at 30s either way -- parsing declarations is not what the build spends its time on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A header that says "using namespace boost::asio" at file scope hands
every translation unit that includes it the whole of Asio in its global
namespace. h1_session.hpp, h2_session.hpp and h2_session_details.hpp did
exactly that -- the last one adding boost::beast on top -- which is what
had been quietly supplying names to their users all along, and what hid
the missing includes fixed in the previous commit.
Qualify with the asio:: alias instead, and let the two sources that were
leaning on the headers (h2_session.cpp, h2_stream.cpp) say what they
mean: h2_session.cpp gets the errc and http aliases h1_session.cpp
already had. The awaitable operators are now pulled in per function,
where the operators are actually used.
server.hpp is a public header, so its chrono_literals directive leaked
into every consumer for the sake of two defaults; spell those out as
std::chrono::hours{24} and std::chrono::seconds{30}. The same directive
in request_handlers.hpp and h2_session.hpp was dead -- neither file uses
a chrono literal.
test_fixtures.hpp keeps its "using namespace asio". It is test-only, it
reaches no further than the test binary, and every test leans on the
unqualified names; naming them one by one there buys nothing. Likewise
the function-local directives: they are scoped, which was never the
problem.
Also drop src/research/sender.cpp, which no target built.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.