Skip to content

#7280: increase retry wait for rmtree #7361

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

Closed
wants to merge 2 commits into from
Closed

#7280: increase retry wait for rmtree #7361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c39c71a
increase retry wait for rmtree to allow more time for virus scanners …
choyrim Nov 15, 2019
cb2aa48
add news item as requested by pull doc.
choyrim Nov 15, 2019
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
1 change: 1 addition & 0 deletions news/7280.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Increase retry wait for rmtree to allow more time for virus scanners to release files on windows.
4 changes: 2 additions & 2 deletions src/pip/_internal/utils/misc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,8 @@ def get_prog():
return 'pip'


# Retry every half second for up to 3 seconds
@retry(stop_max_delay=3000, wait_fixed=500)
# Retry every half second for up to 12 seconds
@retry(stop_max_delay=12000, wait_fixed=500)
def rmtree(dir, ignore_errors=False):
# type: (str, bool) -> None
shutil.rmtree(dir, ignore_errors=ignore_errors,
Expand Down
6 changes: 3 additions & 3 deletions tests/unit/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,11 +366,11 @@ def test_rmtree_retries(tmpdir, monkeypatch):
rmtree('foo')


def test_rmtree_retries_for_3sec(tmpdir, monkeypatch):
def test_rmtree_retries_for_12sec(tmpdir, monkeypatch):
Copy link
Member

Choose a reason for hiding this comment

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

Does this test actually last 12 seconds?

Copy link
Author

Choose a reason for hiding this comment

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

yes, sadly.

>tox -e py37 -- tests\unit\test_utils.py
. . .
py37 run-test: commands[0] | pytest --timeout 300 'tests\unit\test_utils.py'
============================= test session starts =============================
platform win32 -- Python 3.7.4, pytest-3.8.2, py-1.8.0, pluggy-0.13.0
. . .
tests\unit\test_utils.py .............................s................. [ 32%]
........................................................................ [ 82%]
..........................                                               [100%]
=========================== short test summary info ===========================
SKIP [1] tests\unit\test_utils.py:400: condition: sys.platform == 'win32'

=================== 144 passed, 1 skipped in 15.06 seconds ====================
_______________________________________________________________ summary _______________________________________________________________
  py37: commands succeeded
  congratulations :)

Copy link
Member

Choose a reason for hiding this comment

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

Ouch. Can we refactor like this:

def rmtree_provider(**kwargs):
    @retry(**kwargs)
    def rmtree(...):
        # as before
        ...

   return rmtree

and just test that it works as expected with 1 or 2 seconds, then set rmtree = rmtree_provider(stop_max_delay=12000, wait_fixed=500) in pip._internal.utils.misc? Then we're free to adjust the time without impacting test duration.

If you want to make that as a separate PR it would help separate this side concern from your overall proposal to move to 12s.

"""
Test pip._internal.utils.rmtree will retry failures for no more than 3 sec
Test pip._internal.utils.rmtree will retry failures for no more than 12 sec
"""
monkeypatch.setattr(shutil, 'rmtree', Failer(duration=5).call)
monkeypatch.setattr(shutil, 'rmtree', Failer(duration=14).call)
with pytest.raises(OSError):
rmtree('foo')

Expand Down