search

Azure / msrest-for-python

The runtime library "msrest" for AutoRest generated Python clients.
MIT License
41 stars 64 forks source link

Fix incorrect Adapter life cycle #29

Closed lmazuel closed 7 years ago

lmazuel commented 7 years ago

In current code, we create a requests.HTTPAdapter and assign it at instance variable of a client. At the same time, we create one session each time we have a request to do and we assign this adapter. However, look at requests code, the Session object considers he has ownership of the HTTPAdapter. In some situation, the Session object it then closing the HTTPAdapter. Since we keep the adapter as an instance variable, if a session closes an adapter, the new session will get the same adapter and this exception will be raised:

raise ClosedPoolError(self, "Pool is closed.")

This fixes https://github.com/Azure/azure-sdk-for-python/issues/1062

Note also that this PR fixes https://github.com/Azure/msrest-for-python/issues/4 and removes the "hook" feature and replace the few usage by the official requests.hooks feature (this requires too much extra work to keep our "hooks" and fix the adapter at the same time).

Note finally that the Autorest tests part of this PR will break until this PR is merged: https://github.com/Azure/autorest/pull/2143

codecov-io commented 7 years ago

Codecov Report

Merging #29 into master will decrease coverage by 0.52%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
- Coverage   88.86%   88.33%   -0.53%     
==========================================
  Files          10       10              
  Lines        1239     1140      -99     
==========================================
- Hits         1101     1007      -94     
+ Misses        138      133       -5
Impacted Files Coverage Δ
msrest/service_client.py 95.65% <100%> (-0.11%) :arrow_down:
msrest/pipeline.py 99.1% <100%> (+1.54%) :arrow_up:
msrest/__init__.py 60% <0%> (+1.66%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6042cbd...e739f0d. Read the comment docs.