psf / cachecontrol

The httplib2 caching algorithms packaged up for use with requests.
Other
470 stars 123 forks source link

Race condition in FileCache that can result in empty body 200 response #332

Closed thatch closed 5 days ago

thatch commented 6 months ago

I don't have a test case or suggested fix (yet), but documenting that this exists:

  1. Poetry (unlike pip) uses cachecontrol's FileCache with no modifications.
  2. If there is already a cached response, and two different processes (or threads too, probably) revalidate at the same time getting a 304, the first one goes through _cache_set and the second one can find no body (returning a response with empty body because _secure_open_write deletes before creating).

Potential ideas:

woodruffw commented 6 months ago
  • Stop using bespoke mkstemp that has a race, use os.replace. Pro: less code, file isn't missing during the set, Con: commit history doesn't make it obvious why this exists, seems defensive against problems in world-writable cache dirs or maybe a windows issue?

This seems like a clean approach to me. I don't have any additional insight to add on why we don't already use os.replace, except that it might be because os.replace wasn't added until 3.3 and the older os.rename doesn't have the same consistent POSIX-esque semantics on Windows (whereas os.replace does).