homepage

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 )

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

LGTM

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

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

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

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

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

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.

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 

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