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.

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

race condition in pathlib mkdir with flags parents=True #73880

Closed
whitespacer mannequin opened this issue Mar 2, 2017 · 14 comments
Closed

race condition in pathlib mkdir with flags parents=True #73880

whitespacer mannequin opened this issue Mar 2, 2017 · 14 comments
Labels
3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@whitespacer
Copy link
Mannequin

whitespacer mannequin commented Mar 2, 2017

BPO 29694
Nosy @arigo, @berkerpeksag, @serhiy-storchaka, @Mariatta, @whitespacer
PRs
  • bpo-29694: race condition in pathlib mkdir with flags parents=True #1089
  • [3.6] bpo-29694: race condition in pathlib mkdir with flags parents=T… #1126
  • [3.5] bpo-29694: race condition in pathlib mkdir with flags parents=T… #1127
  • Files
  • x1.diff
  • x2.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2017-04-14.02:27:57.369>
    created_at = <Date 2017-03-02.13:53:33.844>
    labels = ['3.7', 'type-bug', 'library']
    title = 'race condition in pathlib mkdir with flags parents=True'
    updated_at = <Date 2017-04-14.02:27:57.368>
    user = 'https://github.com/whitespacer'

    bugs.python.org fields:

    activity = <Date 2017-04-14.02:27:57.368>
    actor = 'Mariatta'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-04-14.02:27:57.369>
    closer = 'Mariatta'
    components = ['Library (Lib)']
    creation = <Date 2017-03-02.13:53:33.844>
    creator = 'whitespacer'
    dependencies = []
    files = ['46707', '46764']
    hgrepos = []
    issue_num = 29694
    keywords = ['patch']
    message_count = 14.0
    messages = ['288801', '289155', '290087', '290102', '290477', '290723', '291386', '291419', '291527', '291539', '291625', '291637', '291638', '291639']
    nosy_count = 6.0
    nosy_names = ['arigo', 'berker.peksag', 'serhiy.storchaka', 'Christoph B\xc3\xb6ddeker', 'Mariatta', 'whitespacer']
    pr_nums = ['1089', '1126', '1127']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue29694'
    versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

    @whitespacer
    Copy link
    Mannequin Author

    whitespacer mannequin commented Mar 2, 2017

    When pathlib mkdir is called with parents=True and some parent doesn't exists it recursively calls self.parent.mkdir(parents=True) after catching OSError. However after catching of OSError and before call to self.parent.mkdir(parents=True) somebody else can create parent dir, which will lead to FileExistsError exception.

    @whitespacer whitespacer mannequin added 3.7 (EOL) end of life type-bug An unexpected behavior, bug, or error labels Mar 2, 2017
    @arigo
    Copy link
    Mannequin

    arigo mannequin commented Mar 7, 2017

    A different bug in the same code: if someone creates the directory itself between the two calls to self._accessor.mkdir(self, mode), then the function will fail with an exception even if exist_ok=True.

    Attached is a patch that fixes both cases.

    @berkerpeksag berkerpeksag added the stdlib Python modules in the Lib dir label Mar 18, 2017
    @serhiy-storchaka
    Copy link
    Member

    Can you provide tests?

    @arigo
    Copy link
    Mannequin

    arigo mannequin commented Mar 24, 2017

    It's a mess to write a test, because the exact semantics of .mkdir() are not defined as far as I can tell. This patch is a best-effort attempt at making .mkdir() work in the presence of common parallel filesystem changes, that is, other processes that would create the same directories at the same time.

    This patch is by no means an attempt at being a complete solution for similar problems. The exact semantics have probably never been discussed at all. For example, what should occur if a parent directory is removed just after .mkdir() created it?

    I'm not suggesting to discuss these issues now, but to simply leave them open. I'm trying instead to explain why writing a test is a mess (more than "just" creating another thread and creating/removing directories very fast while the main thread calls .mkdir()), because we have no exact notion of what should work and what shouldn't.

    @serhiy-storchaka
    Copy link
    Member

    You can monkey-patch os.mkdir and pathlib._NormalAccessor.mkdir.

    Without tests we can't be sure that the patch fixes the issue and that the bug will be not reintroduced by future changes. Tests are required for this change.

    @arigo
    Copy link
    Mannequin

    arigo mannequin commented Mar 28, 2017

    Changes including a test. The test should check all combinations of "concurrent" creation of the directory. It hacks around at pathlib._normal_accessor.mkdir (patching "os.mkdir" has no effect, as the built-in function was already extracted and stored inside pathlib._normal_accessor).

    @ChristophBddeker
    Copy link
    Mannequin

    ChristophBddeker mannequin commented Apr 9, 2017

    I had a problem that can be solved with the presented change.
    But I had also problems to reproduce it in a small example.
    I am not sure if a test is allowed to depend on external libraries.

    The code at the end executed with
    mpirun -np 3 python test.py
    always breaks with the current code of pathlib and works with the fix.

    Maybe this helps to write a test and shows a usecase where this fix is necessary.

    P.S.: I was not able to produce the error with multiprosessing.

    from mpi4py import MPI
    from pathlib import Path
    import tempfile
    
    comm = MPI.COMM_WORLD
    rank = comm.rank
    size = comm.size
    
    with tempfile.TemporaryDirectory() as tmp_dir:
    
        tmp_dir = comm.bcast(tmp_dir, root=0)
        p = Path(tmp_dir) / 'a' / 'b'
        comm.barrier()
        p.mkdir(parents=True, exist_ok=True)

    @arigo
    Copy link
    Mannequin

    arigo mannequin commented Apr 10, 2017

    Maybe we should review pathlib.py for this kind of issues and first apply the fixes and new tests inside PyPy. That sounds like a better way to get things done for these rare issues, where CPython is understandably reluctant to do much changes.

    Note that the PyPy version of the stdlib already contains fixes that have not been merged back to CPython (or only very slowly), though so far they are the kind of issues that trigger more often on PyPy than on CPython, like GC issues.

    @arigo
    Copy link
    Mannequin

    arigo mannequin commented Apr 12, 2017

    Update: a review didn't show any other similar problems (pathlib.py is a thin layer after all). Applied the fix and test (x2.diff) inside PyPy.

    @arigo
    Copy link
    Mannequin

    arigo mannequin commented Apr 12, 2017

    #1089

    (I fixed the problem with my CLA check. Now https://cpython-devguide.readthedocs.io/pullrequest.html#licensing says "you can ask for the CLA check to be run again" but doesn't tell how to do that, so as far as I can tell, I have to ask e.g. here.)

    @Mariatta
    Copy link
    Member

    New changeset 22a594a by Mariatta (Armin Rigo) in branch 'master':
    bpo-29694: race condition in pathlib mkdir with flags parents=True (GH-1089)
    22a594a

    @Mariatta
    Copy link
    Member

    New changeset cbc46af by Mariatta in branch '3.6':
    [3.6] bpo-29694: race condition in pathlib mkdir with flags parents=True (GH-1089). (GH-1126)
    cbc46af

    @Mariatta
    Copy link
    Member

    New changeset d7abeb7 by Mariatta in branch '3.5':
    [3.5] bpo-29694: race condition in pathlib mkdir with flags parents=True (GH-1089). (GH-1127)
    d7abeb7

    @Mariatta
    Copy link
    Member

    I merged Armin's PR and backported tp 3.5 and 3.6.
    Closing this now.
    Thanks all :)

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants