Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Close a potentially unclosed body file before it can interfere with writing a new one #343

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Close a potentially unclosed body file before it can interfere with writing a new one #343

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions cachecontrol/controller.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,9 +277,11 @@ def cached_request(self, request: PreparedRequest) -> HTTPResponse | Literal[Fal

def conditional_headers(self, request: PreparedRequest) -> dict[str, str]:
resp = self._load_from_cache(request)
new_headers = {}
if not resp:
return {}
with resp:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm scratching my head why unraisablehook isn't detecting this missing with. Perhaps requests is closing the connection on GC without issuing a ResourceWarning?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also possible a SeparateBodyCache isn't used in the tests (I haven't checked)?

Copy link
Author

@A5rocks A5rocks Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think test_body_actually_stored_separately doesn't test the case where theres an outdated entry in cache, which is when a ResourceWarning should appear.

Otherwise, conditional_headers either:

  • doesn't have a resp (so no body file to warn)
  • gets shortcircuited around. see

    cachecontrol/cachecontrol/adapter.py

    Lines 70 to 74 in 34564d8

    if cached_response:
    return self.build_response(request, cached_response, from_cache=True)
    # check for etags and add headers if appropriate
    request.headers.update(self.controller.conditional_headers(request))

though I'm not completely sure, cause test_update_cached_response_with_valid_headers_separate_body looks like it tests that case...

new_headers = {}

if resp:
headers: CaseInsensitiveDict[str] = CaseInsensitiveDict(resp.headers)

if "etag" in headers:
Expand All @@ -288,7 +290,7 @@ def conditional_headers(self, request: PreparedRequest) -> dict[str, str]:
if "last-modified" in headers:
new_headers["If-Modified-Since"] = headers["Last-Modified"]

return new_headers
return new_headers

def _cache_set(
self,
Expand Down