Repository navigation
kj-hyper: hyper, rustls and kj's WebSockets behind kj-typed seams - #7535
Conversation
|
@danlapid Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
I'm Bonk, and I've done a quick review of your PR. Adds the
Posted seven inline comments with suggestions. |
016bd15 to
d624654
Compare
|
Since last review: 0 resolved, 1 still open, 0 new. Adds a Unix-only guard to the Not re-run: kj-style (no author changes in their files since the last review) Specialist findings not posted (1)
Reviewed commit: 1a44543d · github run |
d624654 to
6307933
Compare
6307933 to
1564a3b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7535 +/- ##
==========================================
- Coverage 38.63% 38.63% -0.01%
==========================================
Files 868 868
Lines 268564 268575 +11
Branches 25399 25398 -1
==========================================
- Hits 103754 103751 -3
- Misses 150575 150588 +13
- Partials 14235 14236 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2f45dc1 to
4159b41
Compare
4159b41 to
159bd71
Compare
159bd71 to
f17efcc
Compare
guybedford
left a comment
There was a problem hiding this comment.
Checked out f17efcce; //src/rust/cxx/kj-hyper/..., //src/rust/cxx/kj-rs-io/... and //src/rust/kj/... all pass locally (13 targets). Read through server.rs, client.rs, io.rs, body.rs, ffi.rs, tls.rs, kj-hyper.c++ and the four crate patches. The Borrowing lifetimes, the zero-write DISCONNECTED path, SharedWakers, the Head ordering/claim logic, the header-block bounds checks, the pinned-verifier fallback and the patches all look right to me.
I spent most of the time on serve_connection's cancellation model, since that is the one promise in the docs I could not find implemented. Two findings inline on server.rs (one with a repro and a prototype fix that keeps all 58 contract tests green), plus a doc nit on AGENTS.md. None of it is blocking for a crate nothing uses yet, but the first one matters for the server PR that follows.
f17efcc to
d15887c
Compare
d15887c to
fdf033d
Compare
guybedford
left a comment
There was a problem hiding this comment.
Residual considerations for a completed request's finalization tail:
Fine to keep: Tail after a Connection: close response can't be RST-cancelled. This is likely semantically better than kj finalization since it shouldn't be cancellable once the response is fully sent.
Possible follow-on: calls.clear() on a hyper Err can cancel a finished request's tail on a pipelined parse error or a 15s header timeout. Marginally worse than current situation, but pathological case. Principled fix is to clear only calls whose response hasn't completed as a follow-up.
mikea
left a comment
There was a problem hiding this comment.
I took a look at larger code structure/organization and prolific usage of cells is the only thing that really caught my eye.
Of course, if possible, we should get rid of them, but where we can't we need to be thinking more about documenting/enforcing contracts better since we loose all compiler support once we go this way.
It doesn't seem like this could break anything so I'm ok with just giving it a go functionality-wise.
5e47c8a to
d9de001
Compare
179fb93 to
5fd5152
Compare
kj-hyper is the HTTP/1.1, TLS and WebSocket layer for a Rust server whose handlers are kj HTTP interfaces. `server::serve_connection` serves one accepted tokio connection with hyper and dispatches each request to a `Handler` that takes exactly what `kj::HttpService::request` takes (kj headers, a `kj::AsyncInputStream` body, a `kj::HttpService::Response`); CONNECT reaches `Handler::connect` as a `server::Connect` that is answered in Rust or handed to C++ as a `kj::HttpService::ConnectResponse`. `client::Client` implements the kj `Service` trait over hyper-util's pooled legacy client: `Client::new(table, settings, peer, dial)` to one peer through a dialer, asked for paths (`Peer::Origin`) or, of an HTTP proxy, for whole URLs (`Peer::Proxy`), with `tunnel(host)` an HTTP CONNECT through it; `Client::internet(table, settings, tls, connect)` to whatever authority a request's URL names over the caller's `connect(host, port)` (`client::connect_allowed` resolves and connects to the first address a filter admits that accepts), with startTls upgrade through kj's `TlsStarterCallback`. `tls` builds rustls client and server configs from workerd's `TlsOptions` (keypair, trusted certificates, the platform's browser CAs, client-certificate requirement, minimum version). Its cryptography is ring's, except the ECDSA signatures ring cannot verify and BoringSSL can (a P-521 key, or SHA-512 with a P-256 or P-384 key), which the BoringSSL workerd already links verifies (`ecdsa_verify` in kj-hyper.c++), so a chain through a P-521 CA verifies as under kj-http. WebSockets are kj's: kj-hyper does the RFC 6455 handshake and the permessage-deflate agreement (kj's own parser, in kj-hyper.c++) and hands the upgraded transport to `kj::newWebSocket`. A handshake with an unsupported `Sec-WebSocket-Version` is refused with `426` naming version 13 (kj: `400`), and one by POST with `400`. Tokio streams cross to C++ as `kj::AsyncIoStream` (`into_kj_stream`, `into_kj_stream_with`). kj objects that keep a reference into Rust-owned state -- `kj::HttpHeaders` into their header table, a server response or WebSocket into the `WebSocketErrorHandler` in the settings -- cross the bridge as `ffi::Borrowing<'a, T>`, a `KjOwn` bounded by that borrow; the raw bridge functions that build them are `unsafe` and called only from its wrappers. A transport comes with its `Hangup`, a future that resolves when kj's own stream over it would resolve `whenWriteDisconnected()` (a socket hung up or failed, `kj_rs_io::when_write_disconnected`; not a peer that shut down its side; never on Windows, as under kj). `serve_connection` takes it as an argument, and a dialer returns a socket, whose signal `client::Dialed::from` takes, or a `Dialed` made with one. The kj streams made of the connection resolve `whenWriteDisconnected()` from it, and so a `kj::WebSocket` its `whenAborted()`; a stream from plain `into_kj_stream` never does. A socket's signal holds a dup of its descriptor until it is dropped. As kj's `HttpServer` does, `serve_connection` cancels the handler calls of a connection whose transport hangs up (or fails) while it is served; otherwise a call runs to completion, past its response. Messages go out as kj writes them (`body::Head`): the header table's headers first, in the table's order and spelling, then the rest as added, with the connection-level headers, the framing and `Connection: close` set by the protocol at kj's position, so hyper adds none of its own for those. Everything else is hyper's, request parsing included, and differs from kj-http in places: a header's repeated values are written together; a response to HEAD never says `Transfer-Encoding`; a GET or HEAD request body of unknown length is not sent (kj chunks one whose headers say `Transfer-Encoding`); the connection closes after a request that says `Connection: close` (hyper then says so itself, after the other headers) or is HTTP/1.0 without keep-alive; a request with both `Content-Length` and `Transfer-Encoding` is read as chunked and closes the connection; and a request hyper cannot parse never reaches the `Handler` but is answered `400`, `414` or `431` with hyper's description of the error as the body. A call that fails before responding is answered with a bare `500 Internal Server Error` that closes the connection: an exception's text, which kj's default error handler sends, never reaches a client. DISCONNECTED gets no answer, and a failure after the response head went out drops the connection. The prerequisites it needs from the neighbouring crates come along: kj-rs-io's `when_write_disconnected(&socket)` and a public `exception_type` (the type kj gives an I/O error, which `io_kj_error` applies), and in the `kj` crate `ServiceResponse::accept_websocket` with an opaque `kj::WebSocket`, `HeaderTable::builtin()`, and `AsyncInputStream::try_read` / `try_get_length`. The contract tests in kj-hyper/tests are ported from kj's HTTP and TLS tests: a C++ `kj::HttpService` served by kj-hyper and kj-hyper's client behind `kj::newHttpClient`, checked on the wire over loopback TCP and in-process pipes (plain and TLS), driven by a `kj_test` on the tokio-backed `kj::setupAsyncIo()`. They also check header order on the wire, a proxy's whole URLs, and a WebSocket's `whenAborted()` on a reset. TLS policy and unit tests live beside the modules. Nothing in the workerd binary uses the crate yet; workerd's Rust server, in a following change, does. The bridge declarations use the `async unsafe fn f<'a>` form the in-tree cxx requires for borrowed async arguments; they move to the safe form once the bridge supports it. Crate patches (patches/rust/crates/<crate>/, applied through rust.MODULE.bazel): `http` and `httparse` accept the header-value bytes kj accepts (only NUL, CR and LF are rejected), so values round-trip byte for byte; hyper's `HeaderCaseMap` is public so header names keep the application's spelling; hyper's graceful shutdown keeps a connection with a partially received request open until it is answered, as kj's `drain()` does; hyper's client keeps a connection out of the pool until its response body is consumed, as kj's client does; and the response hyper's server generates for a request it cannot parse explains the error in a text/plain body, as kj's server does and RFC 9110 section 15.5 asks. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
5fd5152 to
1a44543
Compare
PR 4 of the Rust-server split: the
kj-hypercrate and the prerequisites it needs, cut fromorigin/main. Nothing in theworkerdbinary uses it yet; workerd's Rust server (a following PR) does.What the crate is
src/rust/cxx/kj-hyper: HTTP/1.1, TLS and WebSockets for a Rust server whose handlers are kj HTTP interfaces.server::serve_connection(io, hangup, &HeaderTable, Rc<ServerSettings>, Rc<dyn Handler>, &Shutdown)serves one accepted tokio connection with hyper.Handler::requesttakes exactly whatkj::HttpService::requesttakes (kj headers, akj::AsyncInputStreambody, akj::HttpService::Response);Handler::connectgets aserver::Connectanswered in Rust (accept()/reject()) or handed to C++ (into_kj()).client::Clientimplements the kjServicetrait over hyper-util's pooled legacy client.Client::new(table, settings, peer, dial)is a client of one peer through a dialer: aPeer::Originis asked for paths, aPeer::Proxy(an HTTP proxy) for absolute URLs sent whole, andtunnel(host)is an HTTP CONNECT through that peer, returned as akj::AsyncIoStream.Client::internet(table, settings, tls, connect)reaches whatever authority a request's URL names over the caller'sconnect(host, port);client::connect_allowed(host, port, allow)is the usual one (resolve, then the first addressallowadmits that accepts). startTls upgrade goes through kj'sTlsStarterCallback.tlsbuilds rustls client/server configs from workerd'sTlsOptions(keypair, trusted certificates, platform browser CAs viarustls-platform-verifier, client-certificate requirement, minimum version) and runs the handshakes. The cryptography is ring's, except the ECDSA signatures ring cannot verify and BoringSSL can (a P-521 key, or SHA-512 with a P-256 or P-384 key): those are verified by the BoringSSL workerd already links (ecdsa_verifyin kj-hyper.c++), in certificate chains and handshakes, so a chain through a P-521 CA verifies as it did under kj-http.kj-hyper.c++), then hands the upgraded transport tokj::newWebSocket. Where the handshake differs from kj: an unsupportedSec-WebSocket-Versionis refused with426andSec-WebSocket-Version: 13(kj:400), and a handshake by POST with400.Hangup: a future that resolves when kj's own stream over that transport would resolvewhenWriteDisconnected()(a socket hung up or failed,kj_rs_io::when_write_disconnected; not a peer that shut down its side; never on Windows, as under kj).serve_connectiontakes it as an argument; a dialer returns a socket, whose signalclient::Dialed::fromtakes, or aDialedmade with one (Dialed::with_hangup, kept acrossDialed::tls). The kj streams made of the connection (a served WebSocket or CONNECT tunnel,server::Upgrading::into_kj(), the client's WebSockets and tunnels,Dialed::into_kj()) resolvewhenWriteDisconnected()from it, and so akj::WebSocketitswhenAborted(), which is how kj learns of a peer that goes away while nothing is read. A socket's signal holds a dup of its descriptor until it is dropped.into_kj_stream/into_kj_stream_withgive tokio streams to C++ askj::AsyncIoStream; the second takes the transport's hang-up signal, the first never resolveswhenWriteDisconnected().body::Head) in the orderkj::HttpHeaders::forEachyields: the header table's first, in the table's order and spelling, then the rest as added. The connection-level headers are the protocol's, not the application's, as under kj'sconnectionHeaders, andHeadsets the framing (Content-Length/Transfer-Encoding: chunked) andConnection: close(a drain, a failed call's answer, a refused CONNECT) itself at kj's position, so hyper adds none of its own for those; theConnection: closehyper sends when the request asked for the close goes after the other headers.500 Internal Server Error, the status text as the body andConnection: close: an exception's text never reaches a client (kj's defaultHttpServerErrorHandlersends it, with 503 or 501 by exception type). DISCONNECTED gets no answer, a failure after the response head went out drops the connection, and a call that returns without responding gets kj's plain-text 500.Content-LengthandTransfer-Encodingis read as chunked, reaches the handler without itsContent-Length, and closes the connection;Content-Lengthor differing duplicates,Transfer-Encodingon HTTP/1.0 and aTransfer-Encodingthat does not end inchunkedare refused with400;Transfer-Encoding: gzip, chunkedand an empty line before the request line are accepted;Handler: hyper answers400(414for a target past 65534 bytes,431for too large a head or more than 16384 headers) withConnection: closeand its own description of the error as atext/plainbody;Connection: closeor is HTTP/1.0 without keep-alive, and an HTTP/1.0 request is answered as HTTP/1.0 and unchunked;Transfer-Encoding, and a GET or HEAD request body of unknown length is not sent (kj chunks it when the request's headers sayTransfer-Encoding).The Rust-facing rules (threads, lifetimes, cancellation, hang-ups, header order, where parsing differs from kj, dropped kj settings) are in the crate docs (
kj-hyper/lib.rs) and the module docs;src/rust/cxx/AGENTS.mdpoints there.Prerequisites included
when_write_disconnected(&socket), the hang-up signal of a socket that stays with its Rust owner (it dups the socket; never resolves on Windows); a publicexception_type(thekj::Exception::Typekj gives an I/O error, which kj-hyper'sio_kj_errorapplies).ServiceResponse::accept_websocketwith an opaquekj::WebSocket,HeaderTable::builtin(),AsyncInputStream::try_read/try_get_length, and theKjOwn<HttpServiceResponse>drop impl.Bridge syntax
Main's bridge requires borrowed async
extern "Rust"declarations to beasync unsafe fn f<'a>(...)with one named lifetime, so kj-hyper'sRustIo::writeand the test helpers'listening/handshake/request/connectuse that form (the rest return'staticfutures or take raw pointers and are unchanged). They move to the safe form once #7533 (safe async bridge functions with borrowed arguments) lands.Tests
src/rust/cxx/kj-hyper/tests: contract tests ported from kj's HTTP and TLS tests. A C++kj::HttpServiceis served by kj-hyper and kj-hyper's client sits behindkj::newHttpClient(kj::HttpService&), checked on the wire over loopback TCP and in-process pipes (plain and TLS), driven by akj_teston the tokio-backedkj::setupAsyncIo(). They also check header order and spelling on the wire in both directions, the bare 500, a proxy's absolute URLs, a CONNECT accepted with aContent-Length, and (except on Windows) a WebSocket'swhenAborted()on a peer's reset but not on its shutdown, for the server and the client. Rust unit tests (TLS policy with fixed PEM material, bodies, I/O, handshake, the answers to unparsable requests) are in<module>-test.rsfiles beside their modules.Dependencies and patches
New crates in
deps/rust/Cargo.toml(lock regenerated with therules_rustcargo wrapper):hyper,hyper-util,http,http-body,tower-service,rustls(ring),tokio-rustls,rustls-platform-verifier,rustls-webpki,x509-cert,sha1,base64,rand.deps/rust/BUILD.bazellinks the platform certificate-store frameworks/libs. Patches inpatches/rust/crates/<crate>/, applied viabuild/deps/rust.MODULE.bazel:http/httparse: accept the header-value bytes kj accepts (only NUL, CR, LF rejected) so values round-trip byte-for-byte (WPTheader-values*.any.js).hyper: publicHeaderCaseMap(header names keep the application's spelling); graceful shutdown keeps a partially received request's connection open until answered, as kj'sdrain()does; the client keeps a connection out of the pool until the response body is consumed, as kj's client does; the response the server generates for a request it cannot parse (400, 414, 431) explains the error in atext/plainbody, as kj's server does and RFC 9110 section 15.5 asks (no body when the request line named HEAD).Verification
🤖 Generated with Claude Code