Tweak implementation of throwaway HTTP sessions #7579
Conversation
| @@ -124,7 +124,7 @@ def test_extra_headers_on_http_method_call(self): | |||
| http = RequestsWrapper(instance, init_config) | |||
|
|
|||
| extra_headers = {"foo": "bar"} | |||
| with mock.patch("requests.get") as get: | |||
| with mock.patch("requests.Session.get") as get: | |||
There was a problem hiding this comment.
Changing all these mock calls took a fair amount of tedious work, but I'm not sure if we should do anything about this.
I thought about parametrizing test cases by the HTTP method, since we do basically the same thing for GET, POST, PUT, PATCH, DELETE. But the devil is in the details and maybe we don't exactly do the same thing in each case, though.
Anyway, we can treat any refactor of these big amounts of mocks as a follow-up…
There was a problem hiding this comment.
Agreed with the idea of pushing that change to a separate PR.
Codecov Report
|
| for option, value in iteritems(self.options): | ||
| setattr(session, option, value) |
There was a problem hiding this comment.
Note: these two lines were not called in the default "don't persist" case previously, so walking through if there's any behavior change here…
- Previously, request-level options were built via the call to
.populate_options(), which takes the kwargs passed viahttp.<method>(), updates it withRequestsWrapperoptions, and returns the result. - Now, we do the same thing, except that we initially copy
RequestsWrapperoptions onto theSessioninstance (via the two lines highlighted here).
The copying onto the Session instance and the behavior of .populate_options() is a bit redundant, since we're basically applying defaults twice (once ourselves via .populate_options(), and once via Session defaults, applied by Requests). But it's not a broken thing to do — and in fact, we were already doing this in the persist_connections: true case.
So there doesn't seem to be any behavioral change. Can anyone confirm I'm getting this right?
|
Closing, we'll come back to this later. |
What does this PR do?
Our
RequestsWrapperhas apersist_connectionswitch. When it's off (the default), we use a throw-away session for each request, instead of reusing a sharedSession.This PR changes this throw-away mechanism from using the top-level
requestsnamespace to using the same session than we'd use when persisting connections. This is done via a._create_session()method shared between each code path (persist / don't persist).Motivation
I'm working on adding Unix Domain Socket support to our
RequestsWrapper(marked as TODO in the docs, plus I'm going to need it for IoT Edge #7465).Using
requests-unixsocketthis requires mounting an adapter onto a session, egsession.mount("unix://", UnixAdapter()), and so in the "don't persist" code path we'll need to create a dedicated session rather than the top-levelrequestslibrary.With this new shared
._create_session()logic, adding UDS support will basically amount to editing_create_session()so that it mounts the adapter. This will apply to both "persist" and "don't persist" use cases.In any case, using the exact same logic in each code path seems like the best way forward!
Additional Notes
Review checklist (to be filled by reviewers)
changelog/andintegration/labels attached