-
Notifications
You must be signed in to change notification settings - Fork 167
chore: add HTTPAdapter to request session #1141
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1141 +/- ##
=======================================
Coverage 91.94% 91.95%
=======================================
Files 144 144
Lines 6196 6200 +4
=======================================
+ Hits 5697 5701 +4
Misses 499 499
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3323d34 to
567c1cf
Compare
567c1cf to
2c04ebc
Compare
|
@agateau-gg I updated the PR, I think it should make more sense now. |
| def test_create_session_mounts_http_adapter(): | ||
| """ | ||
| GIVEN create_session is called | ||
| WHEN the session is created | ||
| THEN HTTPAdapter is mounted for both http:// and https:// | ||
| """ | ||
| session = create_session() | ||
|
|
||
| # Verify adapters are mounted | ||
| assert "http://" in session.adapters | ||
| assert "https://" in session.adapters | ||
|
|
||
| # Verify it's an HTTPAdapter (not the default) | ||
| from requests.adapters import HTTPAdapter | ||
|
|
||
| assert isinstance(session.get_adapter("http://example.com"), HTTPAdapter) | ||
| assert isinstance(session.get_adapter("https://example.com"), HTTPAdapter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test does not test much: Session.adapters contains HTTPAdapter instances by default, for both http and https:
>>> from requests import Session
>>> s = Session()
>>> s.adapters
OrderedDict({'https://': <requests.adapters.HTTPAdapter object at 0x78f4aca515b0>, 'http://': <requests.adapters.HTTPAdapter object at 0x78f4ab918e30>})
I think it can be dropped.
Context
A customer reported this issue when scanning with ggshield, on a self-hosted instance (
XXX= host url).What has been done
Adding a
HTTPAdapterto the session configuration to better handle many parallel requests. The risk is that it could put additional pressure on the server. After reading this article, I leftpool_connectionsto default value, as it shouldn't have much of an impact for our case, but increasedpool_maxsizeas we can see in the logs that this is the param that was blocking for the customer.Another option was to increase the 60s timeout, but I'm afraid it would have too much impact. For instance, when a ggshield command fail, it could result in a longer waiting time for the caller to fetch the output.
I'm open to suggestions if you see another way of handling this.
Validation
Validated with the unit test.
PR check list
skip-changeloglabel has been added to the PR.