_RandomNameSequence was added in issue589982 when generator functions were new optional feature. _RandomNameSequence is implemented as an iterator class. Proposed patch simplifies _RandomNameSequence by implementing it as a generator function.

This is a private name, all uses of _RandomNameSequence() need only the support of the iterator protocol.

The downside of this change:

1. The _RandomNameSequence iterator no longer has the rng field. But it isn't used in the stdlib. And since _RandomNameSequence is a private name, it shouldn't be used in third-party code.

2. The result of no longer copyable neither pickleable. This could cause a problem if it is set as an attribute of copyable of pickleable object. But it is used only as function local or module global instances in tempfile and class attribute of in multiprocessing.synchronize.SemLock and multiprocessing.heap.Arena. All these cases don't involved in copying or pickling. And in general copying and pickling the _RandomNameSequence object is a doubtful idea.

3. This makes iterating the _RandomNameSequence iterator slightly (about 6%) slower. Not a big deal, it isn't used in performance critical code. It is possible to speed up the _RandomNameSequence iterator by the same 6% by using functools.partial (functools already is imported in tempfile), but this makes the code slightly less clear, and I choose the simplicity of the code.

Merged in f50354adaaafebe95ad09d09b825804a686ea843.

After reviewing the change, I created the issue #30051: "Document that the random module doesn't support fork".

The following builbot failure is probably related:

http://buildbot.python.org/all/builders/AMD64%20Windows8.1%20Non-Debug%203.x/builds/623/steps/test/logs/stdio

======================================================================
FAIL: test_main (test.test_threadedtempfile.ThreadedTempFileTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\buildarea\3.x.ware-win81-release\build\lib\test\test_threadedtempfile.py", line 58, in test_main
    self.assertEqual(errors, [], msg)
AssertionError: Lists differ: ['Thread-813Traceback (most recent call la[2133 chars]g\n'] != []

First list contains 4 additional elements.
First extra element 0:
'Thread-813Traceback (most recent call last):\n  File "D:\\buildarea\\3.x.ware-win81-release\\build\\lib\\test\\test_threadedtempfile.py", line 38, in run\n    f = tempfile.TemporaryFile("w+b")\n  File "D:\\buildarea\\3.x.ware-win81-release\\build\\lib\\tempfile.py", line 538, in NamedTemporaryFile\n    (fd, name) = _mkstemp_inner(dir, prefix, suffix, flags, output_type)\n  File "D:\\buildarea\\3.x.ware-win81-release\\build\\lib\\tempfile.py", line 246, in _mkstemp_inner\n    name = next(names)\nValueError: generator already executing\n'

Diff is 2461 characters long. Set self.maxDiff to None to see it. : Errors: errors 4 ok 996
Thread-813Traceback (most recent call last):
  File "D:\buildarea\3.x.ware-win81-release\build\lib\test\test_threadedtempfile.py", line 38, in run
    f = tempfile.TemporaryFile("w+b")
  File "D:\buildarea\3.x.ware-win81-release\build\lib\tempfile.py", line 538, in NamedTemporaryFile
    (fd, name) = _mkstemp_inner(dir, prefix, suffix, flags, output_type)
  File "D:\buildarea\3.x.ware-win81-release\build\lib\tempfile.py", line 246, in _mkstemp_inner
    name = next(names)
ValueError: generator already executing

Thread-814Traceback (most recent call last):
  File "D:\buildarea\3.x.ware-win81-release\build\lib\test\test_threadedtempfile.py", line 38, in run
    f = tempfile.TemporaryFile("w+b")
  File "D:\buildarea\3.x.ware-win81-release\build\lib\tempfile.py", line 538, in NamedTemporaryFile
    (fd, name) = _mkstemp_inner(dir, prefix, suffix, flags, output_type)
  File "D:\buildarea\3.x.ware-win81-release\build\lib\tempfile.py", line 246, in _mkstemp_inner
    name = next(names)
ValueError: generator already executing

Thread-819Traceback (most recent call last):
  File "D:\buildarea\3.x.ware-win81-release\build\lib\test\test_threadedtempfile.py", line 38, in run
    f = tempfile.TemporaryFile("w+b")
  File "D:\buildarea\3.x.ware-win81-release\build\lib\tempfile.py", line 538, in NamedTemporaryFile
    (fd, name) = _mkstemp_inner(dir, prefix, suffix, flags, output_type)
  File "D:\buildarea\3.x.ware-win81-release\build\lib\tempfile.py", line 246, in _mkstemp_inner
    name = next(names)
ValueError: generator already executing

Thread-823Traceback (most recent call last):
  File "D:\buildarea\3.x.ware-win81-release\build\lib\test\test_threadedtempfile.py", line 38, in run
    f = tempfile.TemporaryFile("w+b")
  File "D:\buildarea\3.x.ware-win81-release\build\lib\tempfile.py", line 538, in NamedTemporaryFile
    (fd, name) = _mkstemp_inner(dir, prefix, suffix, flags, output_type)
  File "D:\buildarea\3.x.ware-win81-release\build\lib\tempfile.py", line 246, in _mkstemp_inner
    name = next(names)
ValueError: generator already executing

Yes, it is related. :(

If there is no easy way to make a generator working in multithread environment we need to revert this change.

Why does _get_candidate_names() need a lock? Can't we produce enough entropy to reduce the risk of duplicate names in two threads? Maybe we should use SystemRandom instead, which would also avoid the need to testing if the pid changed.

With one generator per thread, it would prevent the bug no?

Until a solution is found, I proposed to revert the change to "repair" buildbots.

See also the python-ideas thread: [Python-ideas] Thread-safe generators
https://mail.python.org/pipermail/python-ideas/2017-April/045427.html

New changeset 1e62bf145b4865d03a29a5720a4eb84c321a9829 by Victor Stinner in branch 'master':
bpo-30030: Revert f50354ad (tempfile) (#1187)
https://github.com/python/cpython/commit/1e62bf145b4865d03a29a5720a4eb84c321a9829

Thank you Victor. I was going to do this but was unable to do this because I was out of my developing environment.

I reopen the issue to propose a different approach: https://github.com/python/cpython/pull/1191 replaces Random with SystemRandom and drops get_candidate_names().

@Serhiy: I hesitated to add you as a co-author, but I'm not sure that you like my approach :-) Tell me what do you think. At least, I reused your docstring and your updated unit test!

This isn't a different approach and this doesn't work because generators are not thread-safe.

> Remoe _get_candidate_names() and the global _name_sequence

I understood the rationale behind _get_candidate_names() is to prevent generating the same name when _RandomNameSequence() uses random.Random. Since my change uses random.SystemRandom, I consider that it's now ok to create multiple generators in paralle. Is that rationale correct?

test_threadedtempfile passed on Linux, but randomly failed on Windows. :( And _RandomNameSequence is used not only in tempfile.

I suggest to wait until add a wrapper that makes generators thread-safe. Since it added we could restore the generator implementation of _RandomNameSequence.

On Linux creating a temporary file or directory usually consumes only one random name. But due to a bug on Windows (issue22107) it can consume up to TMP_MAX (2147483647 on Windows) names when called with read-only directory. Generating every name consumes about 16 random bytes. This can exhaust the system entropy and slowdown other applications.

"Generating every name consumes about 16 random bytes. This can exhaust the system entropy and slowdown other applications."

Crys and Alex_Gaynor confirmed me on IRC that these two assumptions are both wrong.

See for example https://www.2uo.de/myths-about-urandom/

Q: But that's good! /dev/random gives out exactly as much randomness as it has entropy in its pool. /dev/urandom will give you insecure random numbers, even though it has long run out of entropy.

A:  Fact: No. Even disregarding issues like availability and subsequent manipulation by users, the issue of entropy “running low” is a straw man. About 256 bits of entropy are enough to get computationally secure numbers for a long, long time. 

--

About performance, well, it's not exactly "wrong" but "inaccurate". Abusing /dev/urandom only hurt other applications which also abuse /dev/urandom. Such use case is very unlikely.

* The bad performance of concurrent /dev/urandom reader was analyzed by an old article of 2014, but see comments:
  http://drsnyder.us/2014/04/16/linux-dev-urandom-and-concurrency.html
* The performance issue was fixed in Linux 4.8, https://github.com/torvalds/linux/commit/1e7f583af67be4ff091d0aeb863c649efd7a9112

No consensus was found on this issue how to "simplify" _RandomNameSequence, so I close the issue.