-
Notifications
You must be signed in to change notification settings - Fork 123
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
enable filterwarnings=['error'] and strict-makers, strict-config #341
base: master
Are you sure you want to change the base?
Conversation
cachecontrol/filewrapper.py
Outdated
@@ -117,3 +117,9 @@ def _safe_read(self, amt: int) -> bytes: | |||
self._close() | |||
|
|||
return data | |||
|
|||
def close(self): |
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.
Not sure I follow this: there's a _close
method too, and it seems to be disjoint in implementation from this one. Does this have an effect on the rest of the changes in this PR?
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.
FileWrapper is assigned as a HTTPResponse._fp
which if it has a close method gets called by HTTPResponse.close
which if the request is streaming needs to be called by using the response as a context manager
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.
The windows issues are new and exciting, I'll have to investigate them later
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.
The _close
is disjoint because it's called when the fp is exhausted and the buf has finished being written to, CallbackFileWrapper.close is called when the consumer of the response failed reading the body in some way, if the response was streaming
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.
Does this have an effect on the rest of the changes in this PR?
It's needed otherwise you get a ResourceWarning that's propagated as an unraisable exception
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.
I resolved the windows issues - turns out there's a bug in cpython! I think I've found a cpython bug every time I've added filterwarnings=["error"] to a project!
vars(s).clear() # gc does this. | ||
with pytest.raises(AttributeError): | ||
s.x | ||
with s._CallbackFileWrapper__buf: |
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.
it seems NamedTemporaryFile only issues a ResourceWarning on windows python/cpython#126639
No description provided.