Multiple processes competition on read/write update log

[frostming]

Issue

virtualenv/src/virtualenv/app_data/via_disk_folder.py

Lines 129 to 141 in 0cd009b

	def read(self):
	data, bad_format = None, False
	try:
	data = json.loads(self.file.read_text())
	logging.debug("got {} from %s".format(self.msg), *self.msg_args)
	return data
	except ValueError:
	bad_format = True
	except Exception: # noqa
	pass
	if bad_format:
	self.remove()
	return None

When the file is opened for writing in another process, the JSON content can't be parsed, then the file will be removed. However, on Windows the removal will fail since the file is occupied by anther process.

Environment

Provide at least:

• OS: Windows 10

• pip list of the host python where virtualenv is installed:

  appdirs           1.4.4
  argcomplete       1.11.1
  backcall          0.1.0
  better-exceptions 0.2.2
  certifi           2020.4.5.1
  chardet           3.0.4
  colorama          0.4.3
  decorator         4.4.2
  distlib           0.3.1
  filelock          3.0.12
  idna              2.10
  ipython           7.14.0
  ipython-genutils  0.2.0
  jedi              0.17.0
  naipyext          0.5.2
  parso             0.7.0
  pickleshare       0.7.5
  pip               20.1.1
  pipenv            2020.8.13.dev0 d:\workspace\pipenv
  pipx              0.15.1.3
  prompt-toolkit    3.0.5
  Pygments          2.6.1
  requests          2.24.0
  setuptools        46.4.0
  six               1.14.0
  traitlets         4.3.3
  urllib3           1.25.10
  userpath          1.3.0
  virtualenv        20.0.31
  virtualenv-clone  0.5.4
  wcwidth           0.1.9
  wheel             0.34.2

Output of the virtual environment creation

Make sure to run the creation with -vvv --with-traceback:

Traceback (most recent call last):
  File "C:\Users\runneradmin\.virtualenvs\pipenv-6Kr0DpZ2\lib\site-packages\virtualenv\seed\embed\via_app_data\via_app_data.py", line 94, in _get
    do_periodic_update=self.periodic_update,
  File "C:\Users\runneradmin\.virtualenvs\pipenv-6Kr0DpZ2\lib\site-packages\virtualenv\seed\wheels\acquire.py", line 25, in get_wheel
    wheel = from_bundle(distribution, version, for_py_version, search_dirs, app_data, do_periodic_update)
  File "C:\Users\runneradmin\.virtualenvs\pipenv-6Kr0DpZ2\lib\site-packages\virtualenv\seed\wheels\bundle.py", line 20, in from_bundle
    wheel = periodic_update(distribution, for_py_version, wheel, search_dirs, app_data, do_periodic_update)
  File "C:\Users\runneradmin\.virtualenvs\pipenv-6Kr0DpZ2\lib\site-packages\virtualenv\seed\wheels\periodic_update.py", line 41, in periodic_update
    handle_auto_update(distribution, for_py_version, wheel, search_dirs, app_data)
  File "C:\Users\runneradmin\.virtualenvs\pipenv-6Kr0DpZ2\lib\site-packages\virtualenv\seed\wheels\periodic_update.py", line 62, in handle_auto_update
    u_log = UpdateLog.from_dict(embed_update_log.read())
  File "C:\Users\runneradmin\.virtualenvs\pipenv-6Kr0DpZ2\lib\site-packages\virtualenv\app_data\via_disk_folder.py", line 140, in read
    self.remove()
  File "C:\Users\runneradmin\.virtualenvs\pipenv-6Kr0DpZ2\lib\site-packages\virtualenv\app_data\via_disk_folder.py", line 144, in remove
    self.file.unlink()
  File "C:\hostedtoolcache\windows\Python\3.6.8\x64\lib\pathlib.py", line 1284, in unlink
    self._accessor.unlink(self)
  File "C:\hostedtoolcache\windows\Python\3.6.8\x64\lib\pathlib.py", line 387, in wrapped
    return strfunc(str(pathobj), *args)
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\runneradmin\\AppData\\Local\\pypa\\virtualenv\\wheel\\3.8\\embed\\1\\wheel.json'
PermissionError(13, 'The process cannot access the file because it is being used by another process')
RuntimeError: seed failed due to failing to download wheels wheel

To fix the issue, I prefer to change the writing into atomic, that is, before the writing is done, the content should be kept.

If that is an acceptable approach I can send a PR.

[gaborbernat]

To fix the issue, I prefer to change the writing into atomic, that is, before the writing is done, the content should be kept.

If that is an acceptable approach I can send a PR.

Feel free to put in the PR, and can go from there.

[pfmoore]

Why not simply ignore the error if the file can't be removed? Implementing atomic writes seems to be a fairly complex solution and it's not obvious (to me, at least) why it's better than just failing to remove the file here.

[frostming]

@pfmoore This can keep the file readable until the content is changed, instead of returning a None.

I tested my approach and found that it doesn't solve the race issue when run in parallel. On Windows, it may still fail since the file to be overwritten is owned by another process/thread.

So you are right, ignoring the remove failure looks good to me.