Fix request count check for BadHttpMethod

[bdraco]

The error handler was incorrectly checking for the first request of the server instead of the first request for the connection

#10055 (comment)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.76%. Comparing base (1fa237f) to head (f0c647a).
Report is 2 commits behind head on master.

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #10076   +/-   ##
=======================================
  Coverage   98.76%   98.76%           
=======================================
  Files         121      121           
  Lines       36860    36866    +6     
  Branches     4396     4396           
=======================================
+ Hits        36403    36409    +6     
  Misses        311      311           
  Partials      146      146           

Flag	Coverage Δ
CI-GHA	98.64% <100.00%> (+<0.01%)	⬆️
OS-Linux	98.33% <100.00%> (+<0.01%)	⬆️
OS-Windows	96.18% <100.00%> (+<0.01%)	⬆️
OS-macOS	97.44% <100.00%> (+<0.01%)	⬆️
Py-3.10.11	97.28% <100.00%> (-0.01%)	⬇️
Py-3.10.15	97.82% <100.00%> (-0.06%)	⬇️
Py-3.11.10	97.87% <100.00%> (+<0.01%)	⬆️
Py-3.11.9	97.35% <100.00%> (+<0.01%)	⬆️
Py-3.12.7	98.39% <100.00%> (-0.01%)	⬇️
Py-3.13.0	98.39% <100.00%> (+<0.01%)	⬆️
Py-3.9.13	97.21% <100.00%> (+<0.01%)	⬆️
Py-3.9.20	97.74% <100.00%> (-0.05%)	⬇️
Py-pypy7.3.16	97.35% <100.00%> (+<0.01%)	⬆️
VM-macos	97.44% <100.00%> (+<0.01%)	⬆️
VM-ubuntu	98.33% <100.00%> (+<0.01%)	⬆️
VM-windows	96.18% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[codspeed-hq]

CodSpeed Performance Report

Merging #10076 will not alter performance

_{Comparing fix_request_count_check (f0c647a) with master (1fa237f)}

Summary

✅ 44 untouched benchmarks

[Dreamsorcerer]

I'm still not convinced this will behave as desired. Clients that don't use keep alive (whether unsupported, or a proxy doesn't allow it etc.) will never result in the issue getting logged.
While anyone using a reverse proxy like nginx will likely gets loads of logs as the "client" is the proxy server, not the connecting user.

[bdraco]

I'm still not convinced this will behave as desired. Clients that don't use keep alive (whether unsupported, or a proxy doesn't allow it etc.) will never result in the issue getting logged. While anyone using a reverse proxy like nginx will likely gets loads of logs as the "client" is the proxy server, not the connecting user.

If I remember correctly nginx has keep-alive disabled by default for reverse proxying..maybe that has changed https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive

I'm not sure what you are suggesting for the path forward.

[Dreamsorcerer]

I'm still not convinced this will behave as desired. Clients that don't use keep alive (whether unsupported, or a proxy doesn't allow it etc.) will never result in the issue getting logged. While anyone using a reverse proxy like nginx will likely gets loads of logs as the "client" is the proxy server, not the connecting user.

If I remember correctly nginx has keep-alive disabled by default for reverse proxying..maybe that has changed https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive

I think it's still very common to enable keepalive. Regardless, when disabled we'll never log, when enabled we'll almost always log. Neither is the intended behaviour.

I'm not sure what you are suggesting for the path forward.

Other than logging unconditionally as before, I don't have a better suggestion. Maybe when that other issue is fixed, users could customise this with a middleware?

[Dreamsorcerer]

With a middleware they could choose to opt in to this behaviour, they could use the client IP from a trusted proxy header instead of per-connection etc.

[bdraco]

Other than logging unconditionally as before, I don't have a better suggestion. Maybe when that other issue is fixed, users could customise this with a middleware?

Yeah I think customization in #3287 probably the best way to handle this in the longer term.

I think this PR is ok for the short term as its still logged at debug level so it should be discoverable if the user increases the logging level but considering this generates 100+ issues downstream when nothing is actually wrong I'd prefer to keep it at debug level as a log message isn't so valuable when the common case is noise.

[Dreamsorcerer]

Yep, I think probably merge this for now as something reasonable.

I think #3287 will be a breaking change and won't get backported (which is why I put in on the v4 milestone). So, then we can drop the debug logs in v4 when that is ready.

[patchback]

Backport to 3.11: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.11/fad44f6bc96e4f54a5b8e7e30d1e9e01a021bec9/pr-10076

Backported as #10081

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[patchback]

Backport to 3.12: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.12/fad44f6bc96e4f54a5b8e7e30d1e9e01a021bec9/pr-10076

Backported as #10082

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.