Skip to content

Error 400 when upgrade header is not in lowercase - #116

Merged
tarasko merged 4 commits into
masterfrom
bugfix/upgrade_icmp
Sep 15, 2026
Merged

tarasko merged 4 commits into
masterfrom
bugfix/upgrade_icmp

Conversation

@tarasko

@tarasko tarasko commented Sep 15, 2026

Copy link
Copy Markdown
Owner

Also tighten up some loose end.

  • Strict Base64 validation for Sec-WebSocket-Key.
  • Shared strict request/response HTTP header parser.
  • Response header-count limit.
  • Any Transfer-Encoding is rejected for 101 responses, but ignored for non-101 responses.
  • Shared 16 KiB handshake limit renamed to _http_max_size and applied to client responses.
  • Added unit and functional coverage for all new behavior.

Fixes #115

taras added 3 commits September 15, 2026 09:43
  - Shared strict request/response HTTP header parser.
  - Response header-count limit.
  - Any Transfer-Encoding is rejected for 101 responses, but ignored for non-101 responses.
  - Shared 16 KiB handshake limit renamed to _http_max_size and applied to client responses.
  - Added unit and functional coverage for all new behavior.
@codspeed

codspeed Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will degrade performance by 57.9%

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

❌ 3 regressed benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
test_bench_echo[64] 346.4 ms 1,082.1 ms -67.99%
test_bench_echo[8192] 552.3 ms 1,362.8 ms -59.48%
test_bench_echo[32768] 1.1 s 2 s -42.47%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing bugfix/upgrade_icmp (5eae6ff) with master (9a99eae)

Open in CodSpeed

@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.98%. Comparing base (9a99eae) to head (5eae6ff).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
+ Coverage   91.86%   92.98%   +1.12%     
==========================================
  Files          19       19              
  Lines        2777     2796      +19     
  Branches      156      156              
==========================================
+ Hits         2551     2600      +49     
+ Misses        181      151      -30     
  Partials       45       45              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tarasko

tarasko commented Sep 15, 2026

Copy link
Copy Markdown
Owner Author

@codspeedbot why it is slower?

@codspeed

codspeed Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

@codspeedbot why it is slower?

Short answer: it isn't slower. The reported 57.9% degradation is a measurement-environment artifact, not a real regression.

What happened

The base and head runs were measured on different hardware:

Base (9a99eae) Head (5eae6ff)
CPU Intel Xeon 6973P-C AMD EPYC 7763

The Intel runner is roughly 3x faster than the AMD one for these benchmarks, and the base run happened to land on it. That single unlucky pairing produces the whole diff.

Evidence

Looking at recent master runs on the usual hardware, the numbers are stable and match this PR exactly:

Run [64] [8192] [32768]
master dc78590 1.1 s 1.4 s 2.1 s
master 1480d58 1.1 s 1.4 s 2.0 s
master d5eb524 1.1 s 1.4 s 1.9 s
master 9a99eae (Intel runner) 346 ms 552 ms 1.1 s
this PR 5eae6ff 1.1 s 1.4 s 2.0 s

Re-comparing this PR against the previous master commit d5eb524, which ran on matching hardware, gives 3/3 benchmarks unchanged, overall impact -1.2% — i.e. within noise. The 9a99eae base run is the outlier, not the PR.

The diff itself also supports this: all the changes in picows.pyx are in the HTTP handshake parsing path (header parsing, Base64 key validation, size/count limits). The per-message frame read/write hot path exercised by test_bench_echo is untouched, apart from one memmove in the handshake that now uses a precomputed header_size — executed once per connection.

Why this keeps happening here

Two compounding factors:

  1. The benchmarks run in walltime mode on standard hosted runners, whose CPU generation varies between runs. CodSpeed flagged both of these in the report ("Unknown Walltime execution environment" and "Different runtime environments detected").
  2. test_bench_echo uses benchmark.pedantic(rounds=1, iterations=1), so each benchmark is a single sample with stdev = 0. There is no statistical basis to distinguish a real change from runner variance.

If you want these numbers to be actionable rather than advisory, the options are, in order of effectiveness:

  • Run on CodSpeed Macro Runners (bare-metal, pinned hardware), which removes the cross-generation CPU variance entirely.
  • Increase rounds in test_bench_echo so each result carries a distribution instead of one sample.

No code changes were made — there is nothing to fix in this PR. You can safely acknowledge these three regressions on CodSpeed.

Summary
  • Warming up the performance wizard...
  • Investigate reported 57.9% regression — Root cause: the base run (9a99eae) landed on a much faster Intel Xeon 6973P-C runner while the PR head ran on an AMD EPYC 7763. Comparing the head against the previous master commit (d5eb524, matching hardware) shows 3/3 benchmarks unchanged (-1.2%).
  • Handling comment — Comment handling finished successfully

Learnings updated. See what CodSpeed remembers about this repository.

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

@tarasko
tarasko merged commit 7c7e254 into master Sep 15, 2026
32 of 33 checks passed
@tarasko
tarasko deleted the bugfix/upgrade_icmp branch September 15, 2026 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error 400 when upgrade header is not in lowercase

2 participants