Skip to content

The new SeparateBodyFileCache design makes updates/reads non-atomic #324

Open

Description

@itamarst

The problem

Scenario 1

Program crashes after writing metadata but before writing body. Now the cache thinks the body is empty.

Scenario 2

Multiple processes using the same cache directory. Process A writes metadata, but not yet body. Process B gets metadata, then gets body, and now it thinks the body is empty.

Workarounds

For pip I added logic that basically pretends a cache entry is missing if it doesn't have both body and metadata files.

Solutions

Probably need both a new API that involves writing and reading both at once, and an implementation that makes sure that's atomic. See also the somewhat related #325.

Activity

  1. itamarst commented on Oct 18, 2023

    @itamarst
    ContributorAuthor

    This is my fault, by the way... 😢

    We discussed various solutions on the pip side here: pypa/pip#12361

  2. woodruffw commented on Oct 18, 2023

    @woodruffw
    Member

    Thanks for the report!

    Probably need both a new API that involves writing and reading both at once, and an implementation that makes sure that's atomic. See also the somewhat related #325.

    This makes sense to me -- if I'm understanding correctly, this would mean replacing get + get_body with a single get, correct? Along with internal changes to ensure that we synchronize/atomize the two I/O operations?

  3. itamarst commented on Oct 18, 2023

    @itamarst
    ContributorAuthor

    Yeah, plus have a single write API function.

    For cross-file atomicity, writing the body file first might do the trick, if reading code reads the metadata file first. This would allow the file format to stay the same, at least.

    (Would also be nice if write API didn't take the full bytes, since that means you have to keep full response in memory, but that maybe a bit too much work. Or could use the mmap hack again to work around that and accepts buffer API-implementing objects.)

  4. itamarst commented on Oct 18, 2023

    @itamarst
    ContributorAuthor

    Note that a combination of fixing this and #235 in the ways suggested in previous comment, plus os.replace(), might allow removing the need for locking altogether.

    pip doesn't use locking because historic reliance on deprecated lockfile. They'd vendor the newer filelock if need be, but in general seem happier with fewer dependencies since they need to vendor everything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions