Fix rmtree failures on Windows (#1031)

[Pluckerpluck]

Fixes #1031

---

• Added tests for changed code. (N/A)
• Updated documentation for changed code. (N/A)

Existing tests cover the code change + no need for documentation.

If you really want a test that attempts to simulate the flaky behavior for windows I can try to write one, but I can't see any sensible way to do that. So potentially change robust_rmtree to a private function to indicate no need for direct testing.

---

Avoid the use of TemporaryDirectory, instead manually dealing with temporary files to allow custom cleanup.

Retrying shutil.rmtree multiple times avoids the removal race conditions found in Windows causing:

1. Irrelevant errors when other failures cause removal delays
2. Failures during arbitrary installs caused by antivirus software

[absassi]

@sdispater, can we have this fix in the next version? I'm having to manually apply this patch on Poetry, because otherwise I cannot use Poetry in a large project I work on (since there are many dependencies, this error almost always happens in some random package during dependency resolution).

[brycedrennan]

@Pluckerpluck Thanks for contributing to poetry. I like this fix.

Could you please add a test. You should be able to use mocking to simulate the error.

Here is a test example. Feel free to use/modify this.

def test_robust_rmtree(mocker):
    mocked_rmtree = mocker.patch('shutil.rmtree')

    # should retry after exception and then work
    name = tempfile.mkdtemp()
    mocked_rmtree.side_effect = [OSError("Couldn't delete for some reason"), 'goodpath']
    robust_rmtree(name)

    # will keep retrying and then give up
    name = tempfile.mkdtemp()
    mocked_rmtree.side_effect = OSError("Couldn't delete for some reason")
    with pytest.raises(OSError):
        robust_rmtree(name, timeout=0.1)

def robust_rmtree(path, timeout=1):
    """Robustly tries to delete paths.

    Retries several times if an OSError occurs.
    If the final attempt fails, the Exception is propagated
    to the caller.
    """
    next_retry = 0.001
    while next_retry < timeout:
        try:
            shutil.rmtree(path)
            return  # Only hits this on success
        except OSError:
            # Increase the next_retry and try again
            time.sleep(next_retry)
            next_retry *= 2

    # Final attempt, pass any Exceptions up to caller.
    shutil.rmtree(path)

[brycedrennan]

why 2 seconds instead 1 like the python version? Without a strong justification otherwise, I think we should use 1.0

[Pluckerpluck]

I was running 2 seconds simply becuase I felt that it wasn't an unreasonable wait that little extra longer to increase robustness when this is simply gating a crash situation (i.e. the program will error at the end of the timeout if it fails).

CPython is using it in their tests, so obviously they are much more concious of the tradeoff between robustness and speed.

That being said, 1s is almost certainly enough so I'm fine switching it to that. And we can increase it in the future if people still face the issue.

[brycedrennan]

to match the style of the codebase, please put this on a newline

[Pluckerpluck]

I'll make the changes when I get time. Likely this weekend.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[finswimmer]

Hello,

with the changes introduced in d22afa9 there is at least a second try if something went wrong. Maybe that's enough for the problem that this PR is trying to solve?

Closing this for now. @Pluckerpluck please rebase, adopt your changes and leave a comment if you think that this is still neccessary.

Thanks a lot for your contribution! 👍

fin swimmer

[eblis]

This still occurs with Poetry 1.1.12 on Windows :(
Can we get this merged please ?

[antaradas94]

Actually, the error also continues with Poetry 1.3.0 on Windows. 😞

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.