Conversation
…thread-local helper
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3c4a70e78
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Code Review
This pull request introduces support for HTTP/2 server push and HTTP 103 Early Hints. It updates the async handler signature to include InterimResponseSender and ResourcePusher callables, adds necessary configuration and environment variable support, and provides documentation and tests for these new features. I have identified a potential thread-safety issue in the HTTP/1.1 implementation of SendInterimResponse where it lacks the dispatcher-thread-only enforcement required by the callback contract.
|
LGTM |
fix(h2): close three pre-existing technical debt items
Summary
Closes all three items under Pre-existing Technical Debt :
HTTP/2
:authorityvshostdefault-port normalizationexample.comandexample.com:80(http) /example.com:443(https) are now treated as equivalent.103 Early Hints (RFC 8297) for HTTP/1.1 and HTTP/2
Async handlers can emit
InterimResponseSender(...)before the final response.HTTP/2 server push (RFC 9113 §8.4)
Opt-in via
http2.enable_push, with a boundResourcePusheronAsyncHandlerfor async routes andhttp::PushResource()for sync routes.Test count: 372 → 407 (+35)
Full suite is green twice in a row, including stress, race, and timeout suites.
Phase 1 — Item 2: Authority normalization
Task 1: Scheme-aware
AuthorityMatch(server/http2_stream.cc)AuthorityMatchnow strips the default port (:80for http,:443for https) from both sides before comparing.New tests (+8):
Phase 2 — Scaffolding for Early Hints + Push
Task 2: Extend
AsyncHandlersignatureFiles:
include/http/http_callbacks.hinclude/http/http_router.hserver/http_server.ccAdded type aliases:
InterimResponseSenderResourcePusherPhase 3 — Item 3: 103 Early Hints
Task 3: HTTP/1
SendInterimResponseFiles:
include/http/http_connection_handler.hserver/http_connection_handler.ccserver/http_server.ccHttpConnectionHandler::SendInterimResponse(status, headers)serializes HTTP/1.1 interim responses viaSendRaw.final_response_sent_flag (acquire/release) makes cross-thread post-final drops race-safe.> max_header_size) are dropped.New tests (+6):
100-continue → 103 → 200orderingTask 4: HTTP/2
SubmitInterimHeaders+SendInterimResponseFiles:
include/http2/http2_stream.hinclude/http2/http2_session.hserver/http2_session.ccinclude/http2/http2_connection_handler.hserver/http2_connection_handler.ccserver/http_server.ccPer-stream
final_response_submitted_flag is set inSubmitResponseon success;SubmitInterimHeadersrefuses once final has been submitted.Http2Session::SubmitInterimHeadersusesnghttp2_submit_headerswithNGHTTP2_FLAG_NONE(noEND_STREAM) so the same stream carries both the 103 and the 200.Http2ConnectionHandler::SendInterimResponseenforces a dispatcher-thread-only contract; off-thread callers get a warn log + drop (nonghttp2corruption).Added
ConnectionHandler::IsOnDispatcherThread()helper.Forbidden + pseudo-headers are stripped.
Invalid status codes (
< 102,>= 200,101) are rejected.New tests (+7):
103 → 200100-continue + 103 + 200orderingTask 5: Docs
Files:
docs/http.mddocs/http2.mdNew
## Early Hints (103)sections in both docs, with usage examples, contract table, and RFC references.Phase 4 — Item 1: HTTP/2 server push
Task 6: Config plumbing —
http2.enable_pushFiles:
include/config/server_config.hserver/config_loader.ccinclude/http2/http2_session.hserver/http2_session.ccserver/http_server.cctest/http2_test.hHttp2Config::enable_push = false(default OFF).JSON parsing +
REACTOR_HTTP2_ENABLE_PUSHenv override +Serialize().Http2Session::Settings::enable_push;SendServerPrefacebuildsiv[]dynamically:enable_push=false→ advertise{ENABLE_PUSH, 0}on the wireenable_push=true→ omit the entry (RFC 9113 §7: server MUST NOT write1;nghttp2's local default of1applies internally)Plumbed into
h2_settings_at startup andReload()(new connections only — RFC forbidsSETTINGS_ENABLE_PUSHchanges after preface).New tests (+2):
Task 7: Push primitives on
Http2SessionFiles:
include/http2/http2_session.hserver/http2_session.ccPushEnabled()gates on local config and the peer'snghttp2_session_get_remote_settings(SETTINGS_ENABLE_PUSH).CreateServerInitiatedStream(id)skips the incomplete-stream lifecycle so pushed streams are never subject toparse_timeout_secRST orOldestIncompleteStreamStarttracking.EraseStream(id)provides rollback on submit failure.SubmitPushPromise(parent, method, scheme, authority, path, response)performs atomicPUSH_PROMISE + responsesubmission with validation:GET/HEADonlyscheme ∈ {http, https}/GOAWAYSubmit failure triggers best-effort RST +
EraseStreamrollback.Task 8:
PushResource+ server binding + sync-handler helperFiles:
include/http2/http2_connection_handler.hserver/http2_connection_handler.ccinclude/http/http_server.hserver/http_server.ccinclude/http/push_helper.h(new)Http2ConnectionHandler::PushResourceadds dispatcher-thread and shutdown guards, delegates toSubmitPushPromise, incrementslocal_stream_count_, and flushes.SetupH2Handlersnow binds a realpush_resourceclosure (replacing the phase-2 no-op).Sync-handler access is provided via
HttpServer::current_sync_pusher_thread-local, installed aroundrouter_.Dispatchon the H2 sync path with an RAII guard.Free helper
http::PushResource(method, scheme, authority, path, response)reads the thread-local and returns-1with a debug log when called outside a sync H2 dispatch, including any HTTP/1.x handler.Task 9: Push integration test matrix
Files:
test/http2_test.hHttp2TestClientextended with:Response::pushedvectorPUSH_PROMISEtracking viaOnFrameRecv+OnHeaderSetRefusePushes()opt-out for client SETTINGSNew tests (+13):
POSTrejectedftprejectedHEADresponse body suppressedtotal_requestshttp::PushResource()http::PushResource()on H1 returns-1Reload()togglesenable_pushfor new connectionsTask 10: Docs + roadmap + simplify
Files:
docs/http2.mdserver/http_server.cctest/http2_test.hNew
## Server Pushsection indocs/http2.mdcovering:Moved all 3 roadmap items from Pre-existing Technical Debt to Completed with full implementation notes.
Simplify pass:
MakeH2ResourcePusher()factory inhttp_server.ccto remove duplicated closure bodies across async + sync binding sitesHttp2TestClient::CollectCompletedResponse()to remove duplicated wait-loop code fromSendRequest/SendRequestRawFiles changed
include/config/server_config.h,server/config_loader.ccinclude/http/http_callbacks.h,include/http/http_router.h,include/http/http_connection_handler.h,server/http_connection_handler.ccinclude/http2/http2_stream.h,include/http2/http2_session.h,include/http2/http2_connection_handler.h,server/http2_session.cc,server/http2_stream.cc,server/http2_connection_handler.ccinclude/connection_handler.h(newIsOnDispatcherThread()helper)include/http/http_server.h,server/http_server.cc,include/http/push_helper.htest/http2_test.h,test/http_test.h,test/proxy_test.h,test/route_test.hdocs/http.md,docs/http2.mdTest plan
make clean && make -j4 && ./test_runner— 407 tests pass./test_runner— 407 tests pass (stability)./test_runner stress— pass./test_runner race— pass./test_runner timeout— pass./test_runner http2— 66 tests pass (was 51; +8 authority, +7 Early Hints, +2 SETTINGS-wire, +13 push)./test_runner http— +6 H1 Early Hints tests passNew tests exercise
100-continue + 103interleave orderingSETTINGS_ENABLE_PUSHwire format (both modes)total_requests,HEADbody suppression)Reload()togglesenable_pushfor new connectionsSpec references