Rewrite std/net/http into a usable HTTP/1.1 server and client#1156
Open
marcauberer wants to merge 5 commits into
Open
Rewrite std/net/http into a usable HTTP/1.1 server and client#1156marcauberer wants to merge 5 commits into
marcauberer wants to merge 5 commits into
Conversation
The previous `std/net/http` was a stub: it imported `socket_linux` directly (breaking other platforms) and had empty `start()`/`serve()` bodies. This replaces it with a real, platform-independent implementation built on the cross-platform `std/net/socket` abstraction: - HttpHeaders: insertion-ordered, case-insensitive header collection so that serialization is deterministic. - HttpRequest / HttpResponse: value types that can be built, serialized and parsed. setBody keeps the Content-Length header in sync. - parseHttpRequest / parseHttpResponse: socket-free parsers returning Result. - reasonPhrase: canonical status text for common codes. - HttpServer: route table with addRoute, handle() dispatch with a 404 fallback, and serveOnce / serveForever / stop over real sockets. - HttpClient: send / get / post helpers that auto-add the Host header. Also defines the previously-undefined `SockLenT` in socket_windows.spice (the Linux/Darwin variants already define it), which otherwise blocks importing the socket layer on Windows, and updates the std README to describe the new module. Tests: - std/net/http-messages: build/serialize/parse round-trips (socket-free). - std/net/http-server-routing: HttpServer dispatch + 404 fallback (socket-free). - std/net/http-server-client: end-to-end server+client over TCP (disabled, mirrors the existing sockets-basic convention). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Three fixes found via review and CI on the new std/net/http module: - Client socket I/O went to fd 0: openClientSocket constructed the Socket with connFd = 0, but Socket.read/write operate on connFd, so the HTTP client wrote the request to stdin and read the response from stdin. connFd now mirrors sockFd for client sockets (socket_linux/darwin/windows). - Accepted connections were leaked: acceptConnection overwrote connFd without closing the previous one and only sockFd was ever closed. Added Socket.closeConnection() (a no-op for client/unconnected sockets) and call it in HttpServer.acceptAndRespond after responding and on the parse-error path. - socket_windows was missing the entire client path; added connect/inet_addr/ recv/send/closesocket externs plus Socket.read/write/closeConnection and openClientSocket, and switched Socket.close to closesocket. - Spice's parser does not allow a method call on the result of another call (a.b().c()); rewrote the chained calls in http.spice's parse helpers into intermediate locals, which is what caused the CI parser error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Spice's semantic analysis rejects the binary `+` operator on a char and an int (`cast<char>(c + 32)`), which failed CI in asciiToLower. Switch to the compound-assignment idiom already used by std/text/format (`c += <uint>`), which is supported on char operands. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CI surfaced: `Cannot apply '=' operator on types String& and const String&` at splitHeadAndBody's `headSection = raw`. Spice's operator resolution does not handle a reference-typed variable (a `String&` out-parameter) on the left-hand side of `=`/`+=`; value variables and struct fields work fine. - splitHeadAndBody now returns a Pair<String, String> (head, body) instead of writing into two `String&` out-parameters; callers read it via copy-init. - HttpHeaders.appendTo(String& out) -> HttpHeaders.serialize() returning a String built on its own value-typed `result`; HttpRequest/HttpResponse serialize() concatenate that via a local. These are the only two spots that mutated a reference-typed local through an operator; the rest of the parse/serialize paths only assign to value vars and fields. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CI: a struct-literal initialization of the function-pointer field
(`HttpRoute{ ..., handler }`) was rejected with a field-value type mismatch
even though the expected and actual types print identically -- a Spice
type-identity quirk for function pointers in struct literals.
Add an HttpRoute constructor that assigns the handler via `this.handler =
handler`, the same way std/io/cli-option populates its `p(const T&) callback`
field, and build routes with `HttpRoute(method, path, handler)`.
Co-Authored-By: Claude Opus 4.8 <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
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.
What
Rewrites the
std/net/httpmodule from a non-functional stub into a usable, platform-independent HTTP/1.1 implementation, and adds tests.The previous
http.spiceimportedsocket_linuxdirectly (broken on other platforms) and had emptystart()/serve()bodies. It is replaced with an implementation layered on the cross-platformstd/net/socketabstraction.Module
HttpHeaders— insertion-ordered, case-insensitive header collection, so serialization is deterministic.HttpRequest/HttpResponse— value types you can build, serialize and parse.setBodykeepsContent-Lengthin sync. Public data fields plussetHeader/getHeader/hasHeaderhelpers.parseHttpRequest/parseHttpResponse— socket-free parsers returningResult<...>(split on\r\n\r\n, tolerant of header whitespace/case).reasonPhrase(int)— canonical status text for common codes.HttpServer—addRoute(method, path, handler),handle()dispatch with a 404 fallback, andserveOnce/serveForever/stopover real sockets.HttpClient—send/get/post, auto-adding theHostheader.Drive-by fix
socket_windows.spicereferencedSockLenTbut never defined it (the Linux/Darwin variants do), which would block importing the socket layer — and now HTTP — on Windows. Addedtype SockLenT alias int;. Also updatedstd/README.md, which previously described HTTP as a libcurl-based client.Tests
std/net/http-messagesstd/net/http-server-routingHttpServer.handleroute dispatch + 404 fallback (socket-free)std/net/http-server-clientdisabledsockets-basic)The two runnable cases assert throughout and print one deterministic line, so their
cout.outis justAll assertions passed!.Notes for reviewers
The message (de)serialization is intentionally socket-free and is what the runnable tests exercise. Two things worth a careful look since they lean on less-attested language usage:
f<HttpResponse>(const HttpRequest&)used as a struct field (HttpRoute.handler) — procedure-pointer fields (p()) andf<...>(...)pointer params/locals are both attested separately in the tree, but the function-returning variant as a field is new here.http-messages.I was unable to compile locally in my environment, so this has been written/reviewed against existing std patterns but not executed. CI (or a local
spicetestrun) is the real check.