This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: sporadic test_multiprocessing failure
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: brett.cannon, neologix, pitrou, python-dev
Priority: normal Keywords: patch

Created on 2012-02-21 20:32 by pitrou, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
atomic_exists.diff neologix, 2012-02-21 22:02
Messages (11)
msg153898 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-21 20:32
Just saw this on a buildbot:

======================================================================
ERROR: test_rapid_restart (test.test_multiprocessing.WithProcessesTestManagerRestart)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.ochtman-gentoo-amd64/build/Lib/test/test_multiprocessing.py", line 1513, in test_rapid_restart
    queue = manager.get_queue()
  File "/home/buildbot/buildarea/3.x.ochtman-gentoo-amd64/build/Lib/multiprocessing/managers.py", line 666, in temp
    token, exp = self._create(typeid, *args, **kwds)
  File "/home/buildbot/buildarea/3.x.ochtman-gentoo-amd64/build/Lib/multiprocessing/managers.py", line 564, in _create
    conn = self._Client(self._address, authkey=self._authkey)
  File "/home/buildbot/buildarea/3.x.ochtman-gentoo-amd64/build/Lib/multiprocessing/connection.py", line 773, in XmlClient
    import xmlrpc.client as xmlrpclib
  File "/home/buildbot/buildarea/3.x.ochtman-gentoo-amd64/build/Lib/importlib/_bootstrap.py", line 542, in load_module
    return self._load_module(fullname)
  File "/home/buildbot/buildarea/3.x.ochtman-gentoo-amd64/build/Lib/importlib/_bootstrap.py", line 220, in module_for_loader_wrapper
    return fxn(self, module, *args, **kwargs)
  File "/home/buildbot/buildarea/3.x.ochtman-gentoo-amd64/build/Lib/importlib/_bootstrap.py", line 424, in _load_module
    code_object = self.get_code(name)
  File "/home/buildbot/buildarea/3.x.ochtman-gentoo-amd64/build/Lib/importlib/_bootstrap.py", line 529, in get_code
    self.set_data(bytecode_path, data)
  File "/home/buildbot/buildarea/3.x.ochtman-gentoo-amd64/build/Lib/importlib/_bootstrap.py", line 597, in set_data
    _write_atomic(path, data)
  File "/home/buildbot/buildarea/3.x.ochtman-gentoo-amd64/build/Lib/importlib/_bootstrap.py", line 134, in _write_atomic
    fd = _os.open(path_tmp, _os.O_EXCL | _os.O_CREAT | _os.O_WRONLY, 0o666)
FileExistsError: [Errno 17] File exists: '/home/buildbot/buildarea/3.x.ochtman-gentoo-amd64/build/Lib/xmlrpc/__pycache__/__init__.cpython-33.pyc.140684930127088'

(http://www.python.org/dev/buildbot/all/builders/AMD64%20Gentoo%20Wide%203.x/builds/3288 )
msg153900 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-21 20:37
I am proposing the following patch to have a better unique filename:

diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py
--- a/Lib/importlib/_bootstrap.py
+++ b/Lib/importlib/_bootstrap.py
@@ -129,8 +129,8 @@ def _path_absolute(path):
 
 def _write_atomic(path, data):
     """Function to write data to a path atomically."""
-    # id() is used to generate a pseudo-random filename.
-    path_tmp = '{}.{}'.format(path, id(path))
+    # getpid() and id() are used to generate a pseudo-random filename.
+    path_tmp = '{}.{}-{}'.format(path, _os.getpid(), id(path))
     fd = _os.open(path_tmp, _os.O_EXCL | _os.O_CREAT | _os.O_WRONLY, 0o666)
     try:
         # We first write data to a temporary file, and then use os.replace() to
msg153901 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-02-21 21:34
LGTM
msg153904 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2012-02-21 22:02
> I am proposing the following patch to have a better unique
> filename:

It's unneeded, O_EXCL ensures the file won't be corrupted.
It's actually a regression introduced by de6703671386. My bad.
Here's a patch.
msg153906 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-21 22:11
> > I am proposing the following patch to have a better unique
> > filename:
> 
> It's unneeded, O_EXCL ensures the file won't be corrupted.
> It's actually a regression introduced by de6703671386. My bad.
> Here's a patch.

But is there still a reason to use id(path) then?
msg153908 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2012-02-21 22:26
> But is there still a reason to use id(path) then?

IIRC, the reason is to avoid having a stale pyc file indefinitely in
case of crash:
if we always used, let's say, path + '.tmp', if the process crashes
before the rename, then all subsequent attempts to write the bytecode
will fail because of the stale temporary file.
msg153910 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-21 22:36
> IIRC, the reason is to avoid having a stale pyc file indefinitely in
> case of crash:
> if we always used, let's say, path + '.tmp', if the process crashes
> before the rename, then all subsequent attempts to write the bytecode
> will fail because of the stale temporary file.

But that will also fail if id(path) happens to be fairly
deterministic :) I don't know how much deterministic it can be in
practice, that probably depends on the OS and on the code path?
msg153911 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2012-02-21 22:51
> But that will also fail if id(path) happens to be fairly
> deterministic

Well, if you always get the same id(path), then yes, but I really doubt it, especially with CPython where it's the object's address (I guess there's the same chance of having varying IDs in other implementations):
"""
$ ./python -c "import io; print(id(io.__file__))"
3073909832
$ ./python -c "import io; print(id(io.__file__))"
3073963080
"""

In case of multiprocessing there was a collision because the import is done just after fork(), but the id() will likely change eventually.

If we wanted to be 100% bullet-proof, then we'll need something like mkstemp()/NamedTemporaryFile(), which we can't use because of bootstraping issues.
Of course, adding the PID won't hurt (except that it's a syscall, and will pollute strace's output ;-).
msg153959 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-02-22 15:28
2012/2/21 Charles-François Natali <report@bugs.python.org>

But in other VMs id() is simply a number that gets assigned to objects that
is monotonically increasing. So it can be extremely deterministic if the
object creation order is consistent.
msg153984 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2012-02-22 20:03
New changeset 27d31f0c4ad5 by Charles-François Natali in branch 'default':
Issue #14077: importlib: Fix regression introduced by de6703671386.
http://hg.python.org/cpython/rev/27d31f0c4ad5
msg153986 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2012-02-22 20:16
I've committed the fix.
As for improving the randomness of the temporary file name, I'm not against it (I'm just not convinced it's necessary).
History
Date User Action Args
2022-04-11 14:57:27adminsetgithub: 58285
2012-02-22 20:16:09neologixsetstatus: open -> closed
resolution: fixed
messages: + msg153986

stage: commit review -> resolved
2012-02-22 20:03:47python-devsetnosy: + python-dev
messages: + msg153984
2012-02-22 15:28:51brett.cannonsetmessages: + msg153959
2012-02-21 22:51:33neologixsetmessages: + msg153911
2012-02-21 22:36:07pitrousetmessages: + msg153910
2012-02-21 22:26:15neologixsetmessages: + msg153908
2012-02-21 22:11:54pitrousetmessages: + msg153906
2012-02-21 22:02:42neologixsetfiles: + atomic_exists.diff
keywords: + patch
messages: + msg153904
2012-02-21 21:34:41brett.cannonsetmessages: + msg153901
stage: commit review
2012-02-21 20:37:49pitrousetmessages: + msg153900
2012-02-21 20:32:14pitroucreate