Set TCP_NODELAY on Python e2e client sockets - #8410
Conversation
17d0342 to
934c63d
Compare
Eddy Ashton (eddyashton)
changed the title
fae43ba to
a3acc6a
Compare
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The HTTPX backend overrides the wrong httpcore method, so TCP_NODELAY is not applied; an explicit compatible dependency is also needed.
Review effort: Lite
Findings: None
What changed in this PR
Test-infrastructure updates reduce TCP latency for Python e2e clients and stabilize timing-sensitive tests.
Changes:
- Configures
TCP_NODELAYfor HTTPX and raw-socket clients. - Makes ledger chunk generation transaction-driven.
- Raises the forwarding test’s message limit.
| File | Summary |
|---|---|
tests/schema.py |
Forces signatures after each transaction. |
tests/infra/clients.py |
Configures client socket behavior; the HTTPX backend method and explicit httpcore dependency require correction. |
tests/e2e_logging.py |
Raises the node-to-node forwarding message limit. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Re: the Copilot review's "overrides the wrong I traced the actual call path in httpcore 0.16.3 (the version currently resolved): I diffed The review's second point ("an explicit compatible dependency is also needed") was valid, though: |
f04f6fd to
cad5bba
Compare
|
Update on the |
ef05d52 to
329e95b
Compare
httpcore 0.16 (pinned by tests/requirements.txt's httpx[http2]==0.23.*, since later httpx/httpcore versions break other tests) writes request headers and body as two separate socket writes, and its sync backend does not set TCP_NODELAY. With Nagle's algorithm enabled, the second write is held back until the first is ACKed, and CCF's delayed-ACK timer only fires once it has received the full request, adding ~40ms of latency to every POST issued by the test client. Set TCP_NODELAY on the client socket for both HttpxClient (the default test client) and RawSocketClient. CurlClient is unaffected as curl already enables TCP_NODELAY by default. Measured locally: recovery_test dropped from ~195s to ~162s. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TCP_NODELAY (previous commit) removes ~40ms of Nagle/delayed-ACK latency from every client POST, which exposed two pre-existing test tunings that implicitly relied on that latency to bound their timing: - schema_test's "download" sub-test asserted more than 10 ledger chunks after 10 app transactions, relying on CCF's default 1000ms idle signature interval firing a few extra times during bootstrap to pad the count. With faster requests, setup completes quickly enough that this padding disappears. Fixed by setting sig_tx_interval=1 for this sub-test, so chunk count is driven deterministically by transaction count instead of wall-clock time. - e2e_logging's test_long_lived_forwarding intentionally sets a low node_to_node_message_limit to exercise channel key rotation under load; its comment already noted that too high a request rate can make the hard limit trip mid-flight, invalidating in-flight forwarded requests. Faster requests made this race common enough to fail CI consistently. Fixed by increasing message_limit from 30 to 90, giving enough headroom for the higher achievable request rate. Both were confirmed via repeated local A/B runs (with/without the TCP_NODELAY change): reliably reproduced with the fix, reliably passed without it, and reliably pass again with these adjustments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
infra.clients.HttpxClient's _NoDelaySyncBackend relies on httpcore's private internals (module path, class, method and connection-pool attribute names) to set TCP_NODELAY. httpx 0.23.3 only constrains httpcore to >=0.15.0,<0.17.0, so without an explicit pin, a future httpcore release inside that range could change those internals and silently break the patch (pip would still resolve cleanly, but TCP_NODELAY would stop being applied without any test failing). Pin httpcore==0.16.3 (the version httpx 0.23.3 currently resolves to) so the dependency matches what the patch was written and tested against. Verified compatible: `pip install "httpx[http2]==0.23.*" "httpcore==0.16.3"` resolves without conflict, and connect_tcp / _network_backend are unchanged across the entire allowed range (0.15.0 through 0.16.3), so this pin does not change current behaviour - it only prevents silent drift. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
329e95b to
a5850d1
Compare
Stacked on #8416
This PR targets
fix/snapshot-access-races-alternative(#8416), which fixes snapshot selection and subsequent latest-snapshot advancement. The earlier snapshot workaround from this branch has been dropped; the snapshot tests now come entirely from #8416.#8412 has merged into
main. The current #8416 base predates that merge and needs to incorporate it to carry the recovery-test fix into this stack.Motivation
The pinned httpx 0.23 / httpcore 0.16 HTTP/1.1 client writes request headers and body separately without enabling TCP_NODELAY. Nagle's algorithm can hold the body until the server's delayed ACK arrives, adding roughly 40 ms per POST.
Enable TCP_NODELAY on the Python e2e client's sockets to remove that delay and measure the overall CI runtime impact.
Changes
HttpxClientthat enables TCP_NODELAY when connecting. The connection setup is shared by HTTP/1.1 and HTTP/2.RawSocketClient; leaveCurlClientunchanged.Test infrastructure only; no CHANGELOG entry, recovery padding changes, or LoggingTxs refactor. Kept as a draft to assess CI runtime impact.