sporadic test_multiprocessing failure

[pitrou]

BPO	14077
Nosy	@brettcannon, @pitrou
Files	atomic_exists.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2012-02-22.20:16:09.747>
created_at = <Date 2012-02-21.20:32:14.591>
labels = ['type-bug', 'library']
title = 'sporadic test_multiprocessing failure'
updated_at = <Date 2012-02-22.20:16:09.745>
user = 'https://github.com/pitrou'

bugs.python.org fields:

activity = <Date 2012-02-22.20:16:09.745>
actor = 'neologix'
assignee = 'none'
closed = True
closed_date = <Date 2012-02-22.20:16:09.747>
closer = 'neologix'
components = ['Library (Lib)']
creation = <Date 2012-02-21.20:32:14.591>
creator = 'pitrou'
dependencies = []
files = ['24593']
hgrepos = []
issue_num = 14077
keywords = ['patch']
message_count = 11.0
messages = ['153898', '153900', '153901', '153904', '153906', '153908', '153910', '153911', '153959', '153984', '153986']
nosy_count = 4.0
nosy_names = ['brett.cannon', 'pitrou', 'neologix', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue14077'
versions = ['Python 3.3']

[pitrou]

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 )

[pitrou]

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

[brettcannon]

LGTM

[neologix]

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.

[pitrou]

> 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?

[neologix]

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.

[pitrou]

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?

[neologix]

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 ;-).

[brettcannon]

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.

[python-dev]

New changeset 27d31f0c4ad5 by Charles-François Natali in branch 'default':
Issue bpo-14077: importlib: Fix regression introduced by de6703671386.
http://hg.python.org/cpython/rev/27d31f0c4ad5

[neologix]

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).