-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix client connection header not reflecting connector force_close value
#10003
Conversation
It appears this actually does nothing because it checks the existing headers to set the headers
It appears this actually does nothing because it checks the existing headers to set the headers
CodSpeed Performance ReportMerging #10003 will improve performances by 8.52%Comparing Summary
Benchmarks breakdown
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #10003 +/- ##
==========================================
- Coverage 98.73% 98.73% -0.01%
==========================================
Files 121 121
Lines 36738 36726 -12
Branches 4384 4383 -1
==========================================
- Hits 36272 36260 -12
Misses 314 314
Partials 152 152
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Slept on it. Still think it can go. Need to do some git archeology to see why this was added and how we got here in case I'm missing something |
|
I think this was actually supposed to set the |
force_close value
force_close valueforce_close value
Backport to 3.10: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply 78d1be5 on top of patchback/backports/3.10/78d1be5d79f67597313354646eec200ff603fd9d/pr-10003 Backporting merged PR #10003 into master
🤖 @patchback |
Backport to 3.11: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply 78d1be5 on top of patchback/backports/3.11/78d1be5d79f67597313354646eec200ff603fd9d/pr-10003 Backporting merged PR #10003 into master
🤖 @patchback |
Backport to 3.12: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply 78d1be5 on top of patchback/backports/3.12/78d1be5d79f67597313354646eec200ff603fd9d/pr-10003 Backporting merged PR #10003 into master
🤖 @patchback |
…eflecting connector `force_close` value (#10009)
…eflecting connector `force_close` value (#10010)
…eflecting connector `force_close` value (#10008)
The
Connectionheader should be set based on theforce_closevalue in the connector.It appears that the code to set the connection header had no effect because it calls
keep_alivewhich checks the existing connection header and uses the result to set the headers in a block that only ever called when the connection header is not set.AFIACT its not worked as expected since 8e6723a where the
test_http10_keep_alive_defaulttest was marked asxfailandHTTP/1.0connections never set thekeep-aliveheader but still expected to reuse the connection. Additionally ifforce_closewas set on the connector we never told the remote we were going to close the connection by setting the connection header toclosefor theHTTP/1.1case.The code was only being exercised because the test was mocking out
keep_aliveto impossible values that disagreed with the block insendthat was only entered when the connection header was not set.aiohttp/aiohttp/client_reqrep.py
Line 575 in af33a82
It's likely not many people are using
force_closeor setting the http version to 1.0 so the impact is minimal. On the plus side it makes sending requests 8% faster since it's not having to do all the useless checks now