Skip to content

Send headers implicitly in WriteBodyFull - #149

Open
resetius wants to merge 1 commit into
masterfrom
http-write-body-full-headers
Open

resetius wants to merge 1 commit into
masterfrom
http-write-body-full-headers

Conversation

@resetius

@resetius resetius commented Nov 4, 2026

Copy link
Copy Markdown
Owner

Problem

TResponse::WriteBodyFull called without a prior SendHeaders() wrote only the body: the client got hello with no status line and no headers. The docs contradicted each other (some said WriteBodyFull implies SendHeaders, the examples called SendHeaders first).

Fix

If the headers were not sent yet, WriteBodyFull sets Content-Length to the body size (unless Content-Length or Transfer-Encoding is already set) and sends the headers, then the body. If they were already sent, behaviour is unchanged (body only), so code that sets its own Content-Length and calls SendHeaders() first produces the same bytes. Doxygen and the examples now state this one rule.

Tests

Two end-to-end tests through TWebServer over a real socket in tests/test_http.cpp:

  • WriteBodyFull without SendHeaders gives a status line, Content-Length: 5 and the body (fails on master, passes now).
  • SetHeader("Content-Length") + SendHeaders + WriteBodyFull gives the same output with no duplicate headers.

@github-actions

github-actions Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Unit Test Results (MacOS)

147 tests  +1   147 ✅ +1   21s ⏱️ ±0s
  4 suites ±0     0 💤 ±0 
  4 files   ±0     0 ❌ ±0 

Results for commit 6513a4d. ± Comparison against base commit cae2aef.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Unit Test Results (Linux)

180 tests  +1   180 ✅ +1   14s ⏱️ -1s
  4 suites ±0     0 💤 ±0 
  4 files   ±0     0 ❌ ±0 

Results for commit 6513a4d. ± Comparison against base commit cae2aef.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Unit Test Results (Windows)

136 tests  +1   136 ✅ +1   39s ⏱️ ±0s
  4 suites ±0     0 💤 ±0 
  4 files   ±0     0 ❌ ±0 

Results for commit 6513a4d. ± Comparison against base commit cae2aef.

♻️ This comment has been updated with latest results.

@resetius
resetius force-pushed the http-write-body-full-headers branch 3 times, most recently from dac4061 to e5b8226 Compare November 4, 2026 01:38
@resetius
resetius force-pushed the http-write-body-full-headers branch from 50f6fbc to 6513a4d Compare November 4, 2026 02:24
@resetius

resetius commented Nov 4, 2026

Copy link
Copy Markdown
Owner Author

Independent review of head 6513a4d3f31679082aaf127dd0750c6db564044c. Verdict: no P1/P2 findings, OK to merge; P3 items below.

Checked:

  • CI on this head: Linux, macOS, Windows builds and tests pass. Locally (macOS, ASan) ut_test_http passes 3 of 3 runs (14 tests).
  • The new test against master: I built the PR tree with coroio/http/httpd.cpp and httpd.hpp taken from cae2aef and ran the same test_http_write_body_full; it fails ("hello" != "HTTP/1.1 200 OK\r\nContent-Length: 5...", i.e. master sends only the body). With the PR's httpd.cpp it passes. So the implicit case really fails without the fix, and the explicit case (Content-Length + SendHeaders + WriteBodyFull) is byte-identical on both.
  • qumir-service (server.cpp:118, 198, 206, 589, 645) and the qumir_private plugin (plugin/util.cpp:135-136, 143-144) all call SendHeaders() before WriteBodyFull(), so HeadersSent_ is already true and the new branch is skipped: output unchanged byte for byte (server.cpp:118 sets no Content-Length and Connection: close, also untouched).
  • Docs: httpd.hpp class example, SendHeaders and WriteBodyFull comments all describe the new rule consistently; the README snippet (SendHeaders then WriteBodyFull) still works. No example in examples/ uses these calls.

P3:

  • coroio/http/httpd.cpp:370: the "already set" test is case-sensitive because Headers_ is a std::map<std::string, std::string> and SetHeader stores the name as given. Repro (router: SetHeader("content-length","5"); WriteBodyFull("hello")): response has both Content-Length: 5 and content-length: 5 (duplicate header). Same case-sensitivity already exists for the Transfer-Encoding/Connection detection in SendHeaders. Suggest a case-insensitive comparator for Headers_ (or normalising in SetHeader).
  • Transfer-Encoding: chunked plus WriteBodyFull (headers not sent yet): headers are sent without Content-Length, the body goes out as chunks (5\r\nhello\r\n) but no terminating 0\r\n\r\n is written, so the client waits for more. Same as before the PR for an explicit SendHeaders() (the caller has to finish with WriteBodyChunk("", 0)), but the new doc could say so, or WriteBodyFull could write the terminator when it sent the headers itself.
  • Status/method rules are not applied (not worse in bytes than before, but now more visible): a 204 gets Content-Length: 0 (RFC 9110 says a server must not send Content-Length in a 204) and a HEAD request gets the body after the headers. Before the PR the body was written in both cases as well.

@resetius resetius added the agent-reviewed An independent review agent checked this PR; the verdict is in a PR comment label Nov 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-reviewed An independent review agent checked this PR; the verdict is in a PR comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant