Fix issue 7280 - issue warning after retry #7546

danizen · 2020-01-02T21:19:42Z

Adds a new test utility in tests\lib\fileystem.py designed to help create multi-process race conditions of this kind. Tests for test utility are in tests\unit\test_utils_filesystem.py
Tests the race condition with code in tests\unit\test_util_temp_dir.py
Address the issue by catching the error after retry in src\pip\_internal\util\temp_dir.py and issuing a warning.

- Try to delete the temporary directory, as most of the time this works - If it fails on windows due to an Access error, it clearly is not an Access error because we just created the directory. Warn the user and continue.

- Add issue description file

- Provide a class that manages a sub-process which can open a file and monkey with pip's abiliity to remove a file on windows. - This class can function as both a context manager and/or a pytest fixture.

- TempDirectory() tries to delete the directory as normal - if cleanup fails on Windows due to EACCES, warn about virus scanner - This is a more specific error than previous error, but does not change the behavior when a user is attempting to pip uninstall in a system directory - This changes the messaging, but risks leaving the directory behind. - Leaving the directory behind, however, is what is already happening - The fix for pypa#7479 gives the Virus scanner more time to finish its work, but there is still a possibility for a race condition that leaves the impression that there is an error in pip itself.

- IOError persists as an issue in Python 2.7.17 on win32

- TempDirectory() tries to delete the directory as normal - if cleanup fails on Windows due to EACCES, warn about virus scanner - This is a more specific error than previous error, but does not change the behavior when a user is attempting to pip uninstall in a system directory - This changes the messaging, but risks leaving the directory behind. - Leaving the directory behind, however, is what is already happening - The fix for pypa#7479 gives the Virus scanner more time to finish its work, but there is still a possibility for a race condition that leaves the impression that there is an error in pip itself.

danizen · 2020-01-02T21:57:24Z

@pfmoore @chrahunt, I am completely confused about the appveyor failure. This status response from psutil isn't one of the ones present in psutil statuses, and ignoring by adding something like this didn't seem to show the latest code:

latest_children = [child for child in process.children() if child.status() != 'terminated']

It does seem to me to indicate that the exception for a missing file is not properly caught and printed, but I have not been able to reproduce that on my own.

chrahunt

These tests look great, exactly the kind of thing I was talking about in #7280 (comment)!

A few things:

Can we limit this PR to only the Windows/Python API tests? That way we can focus on each one piece individually and not block the additional tests on review of the behavior change in TempDirectory.
Can we add tests involving processes under different users? I think it would be good to try to do it in this PR since I suspect we will need to make some fundamental changes to how we are running the child process in order to accommodate it.
It's generally good to not put the issue number in your commit titles/messages, otherwise it spams the issue.

tools/requirements/tests.txt

chrahunt · 2020-01-03T20:48:59Z

tests/lib/filesystem.py

+            # Now path is open and we wait for signal to exit
+            conn.recv()
+    finally:
+        if f:


Instead of manually tracking the file and connection we can use ExitStack and closing which are available in pip._vendor.contextlib2, like:

with ExitStack() as stack: stack.enter_context(closing(conn)) # ... f = stack.enter_context(open(path, 'r'))

This suggested change did not work for me, and caused the sub-process or main process to hang. In general, I liked it more that the sub-process was using primitive parts of Python, and was independent of pip's own infrastructure, so I thought I'd raise that as a good thing before we potentially figure out what I did wrong :)

tests/lib/filesystem.py

tests/unit/test_utils_filesystem.py

chrahunt · 2020-01-03T21:15:41Z

tests/unit/test_utils_filesystem.py

@@ -59,3 +70,53 @@ def test_copy2_fixed_raises_appropriate_errors(create, error_type, tmpdir):
        copy2_fixed(src, dest)

    assert not dest.exists()
+
+
+def test_file_opener_no_file(process):


These FileOpener tests would be better in tests/lib/test_filesystem.py. This file is really for tests of functionality in pip/_internal/utils/filesystem.py.

chrahunt · 2020-01-03T21:17:48Z

tests/unit/test_utils_filesystem.py

+
+
+@skip_unless_windows
+def test_file_opener_produces_unlink_error(tmpdir, process):


Since these tests are more against Python/Windows than anything in pip/_internal/utils/filesystem.py, I would put them in tests/functional/test_windows.py.

I think moving them to tests/lib/test_lib_filesystem.py is hopefully enough. Since producing these errors was the whole point, I'd rather not separate these tests from the other tests. Please resolve if you think this is OK.

- Remove "action" since currently it is basically unused - Move tests of FileOpener to tests/lib/test_filesystem - Use path.write_text to initialize file - Avoid comment in changes to requirements

- The test should look for the correct error on PY2 or PY3

danizen · 2020-01-06T17:43:50Z

@chrahunt, I've done some of the work to make this pull request only about the new tests, rather than about issue #7280, with the following exceptions:

the new unit test in "tests/unit/tests_util_temp_dir.py" is marked as pytest.mark.xfail - so, I've produced a failing test for TempDirectory that illustrates the issue we are trying to address
there is still news/7280.bugfix, because I haven't quite despaired of addressing the actual issue with this pull request. However, I want to be clearer that the FileOpener does reproduce the issue.

I think I've addressed the issues with the pull you raised. I named the test file as tests/lib/test_lib_filesystem, just because that is clearer to me when I enter pytest -k test_lib, I get all the tests on tests/lib. I am very open to changing that; it just seemed a small bit better to me.

danizen · 2020-01-06T21:52:04Z

I have pull request fatigue. All of this works on my Desktop, both as davisda4 and as a local administrator. I've run tox -e py35, tox -e py36, and tox -e py27.

danizen · 2020-01-06T22:25:07Z

I have pull request fatigue. All of this works on my Desktop, both as davisda4 and as a local administrator. I've run tox -e py35, tox -e py36, and tox -e py27.

I spoke too quickly. I have run these specific unit tests, but I am now re-running the entire suite with Python 3.6 on Windows, and we'll see.

pypa-bot · 2020-01-10T17:01:21Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

pradyunsg · 2020-02-06T06:34:01Z

Hi @danizen! Is there anything we can help you with to move this PR forward?

BrownTruck · 2020-04-13T21:15:08Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

danizen · 2020-04-14T00:28:59Z

Hi @danizen! Is there anything we can help you with to move this PR forward?

@pradyunsg,

I was mostly interested in issue #7280, which we are no longer seeing since pip 20. I think the main work I and the other contributor did is to clarify the issue, but moving the TemporaryDirectory clean-up for the wheel to the end (by making it global), ended up fixing the issue.

As a contractor, I've pushed my government boss as far as I can to get her to let me do open source contributions. The agreement we worked out was that submitting issues and clarifying them is something they'll pay for, but fixing them I'm on my own. So, I think I'm done. There is no reason I would want to make Windows easier and more foolproof except for my employer preferring Windows development boxes.

SanketDG · 2020-04-14T03:24:53Z

Isn't this largely superseded (especially during installing) with #7882?

danizen · 2020-04-14T03:40:20Z

Isn't this largely superseded (especially during installing) with #7882?

@SanketDG,

Looks like - in any case, this pull request is no longer associated with issue #7280, as I was asked to make it only test code.

Dan Davis added 13 commits December 30, 2019 18:21

Addresses pypa#7280 - More targeted fix to problem

ae887ac

- Try to delete the temporary directory, as most of the time this works - If it fails on windows due to an Access error, it clearly is not an Access error because we just created the directory. Warn the user and continue.

Address pypa#7280 - Add issue description file

b90cc2c

- Add issue description file

Hardening test cases for pypa#7280

942ab14

- Provide a class that manages a sub-process which can open a file and monkey with pip's abiliity to remove a file on windows. - This class can function as both a context manager and/or a pytest fixture.

Update news entry for pypa#7280 to be more complete.

b6c0991

Issue pypa#7280 - Clean-up python2.7, isort, and flake8 errors

eb73752

Fix pypa#7280 - address Python2.7 issues resulting from PEP 3151

fc5450f

Fix pypa#7280 - another attempt to satisfy older versions of Python

cb1a787

- IOError persists as an issue in Python 2.7.17 on win32

Issue pypa#7280 - Attempt to address failing python2.7 test

0fb324b

Fix pypa#7280 - make test code more DRY

8770497

Merge branch 'master' of github.com:pypa/pip into 7280-alt-3

68eb8d4

Merge branch '7280-alt-3' of github.com:danizen/pip into 7280-alt-3

d00958d

chrahunt suggested changes Jan 3, 2020

View reviewed changes

Dan Davis added 4 commits January 6, 2020 12:17

Address requests changed in pull request

3ed7733

- Remove "action" since currently it is basically unused - Move tests of FileOpener to tests/lib/test_filesystem - Use path.write_text to initialize file - Avoid comment in changes to requirements

Assert something about the error raised for windows

b0216be

Update test that this produces an Exception

38e6d41

- The test should look for the correct error on PY2 or PY3

Update pull request for linter errors

aadefd6

Clean-up expected errors and exceptions

40bf93a

Be more forceful about terminating a joined thread if it is still alive.

5be7b66

pypa-bot added the needs rebase or merge PR has conflicts with current master label Jan 10, 2020

Dan Davis added 2 commits January 10, 2020 12:04

Restore news/7280.bugfix - we'll need a separate issue to decouple this.

17246e7

Merge branch 'master' of github.com:pypa/pip into 7280-temp-dir

88dffb1

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jan 10, 2020

BrownTruck added the needs rebase or merge PR has conflicts with current master label Apr 13, 2020

danizen closed this Apr 14, 2020

lock bot added the auto-locked Outdated issues that have been locked by automation label May 20, 2020

lock bot locked as resolved and limited conversation to collaborators May 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue 7280 - issue warning after retry #7546

Fix issue 7280 - issue warning after retry #7546

danizen commented Jan 2, 2020

danizen commented Jan 2, 2020

chrahunt left a comment •

edited

Loading

chrahunt Jan 3, 2020

danizen Jan 6, 2020

chrahunt Jan 3, 2020

chrahunt Jan 3, 2020

danizen Jan 6, 2020

danizen commented Jan 6, 2020

danizen commented Jan 6, 2020

danizen commented Jan 6, 2020

pypa-bot commented Jan 10, 2020

pradyunsg commented Feb 6, 2020

BrownTruck commented Apr 13, 2020

danizen commented Apr 14, 2020

SanketDG commented Apr 14, 2020

danizen commented Apr 14, 2020



		@skip_unless_windows
		def test_file_opener_produces_unlink_error(tmpdir, process):

Fix issue 7280 - issue warning after retry #7546

Fix issue 7280 - issue warning after retry #7546

Conversation

danizen commented Jan 2, 2020

danizen commented Jan 2, 2020

chrahunt left a comment • edited Loading

Choose a reason for hiding this comment

chrahunt Jan 3, 2020

Choose a reason for hiding this comment

danizen Jan 6, 2020

Choose a reason for hiding this comment

chrahunt Jan 3, 2020

Choose a reason for hiding this comment

chrahunt Jan 3, 2020

Choose a reason for hiding this comment

danizen Jan 6, 2020

Choose a reason for hiding this comment

danizen commented Jan 6, 2020

danizen commented Jan 6, 2020

danizen commented Jan 6, 2020

pypa-bot commented Jan 10, 2020

pradyunsg commented Feb 6, 2020

BrownTruck commented Apr 13, 2020

danizen commented Apr 14, 2020

SanketDG commented Apr 14, 2020

danizen commented Apr 14, 2020

chrahunt left a comment •

edited

Loading