[issue30030] Simplify _RandomNameSequence

[STINNER Victor]

STINNER Victor added the comment:

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

--
resolution:  -> rejected
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30030] Simplify _RandomNameSequence

[STINNER Victor]

STINNER Victor added the comment:

"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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30030] Simplify _RandomNameSequence

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30030] Simplify _RandomNameSequence

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30030] Simplify _RandomNameSequence

[STINNER Victor]

STINNER Victor added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30030] Simplify _RandomNameSequence

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30030] Simplify _RandomNameSequence

[STINNER Victor]

STINNER Victor added the comment:

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!

--
resolution: rejected -> 
status: closed -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30030] Simplify _RandomNameSequence

[STINNER Victor]

Changes by STINNER Victor :


--
pull_requests: +1319

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30030] Simplify _RandomNameSequence

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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

--
resolution:  -> rejected
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30030] Simplify _RandomNameSequence

[STINNER Victor]

STINNER Victor added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30030] Simplify _RandomNameSequence

[STINNER Victor]

STINNER Victor added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30030] Simplify _RandomNameSequence

[STINNER Victor]

Changes by STINNER Victor :


--
pull_requests: +1316

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30030] Simplify _RandomNameSequence

[STINNER Victor]

STINNER Victor added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30030] Simplify _RandomNameSequence

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Yes, it is related. :(

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30030] Simplify _RandomNameSequence

[STINNER Victor]

STINNER Victor added the comment:

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\nf = 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\nname = 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

--
resolution: fixed -> 
status: closed -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30030] Simplify _RandomNameSequence

[STINNER Victor]

STINNER Victor added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30030] Simplify _RandomNameSequence

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Merged in f50354adaaafebe95ad09d09b825804a686ea843.

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30030] Simplify _RandomNameSequence

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30030] Simplify _RandomNameSequence

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
pull_requests: +1219

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30030] Simplify _RandomNameSequence

[Serhiy Storchaka]

New submission from Serhiy Storchaka:

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

--
components: Library (Lib)
messages: 291425
nosy: haypo, pitrou, rhettinger, serhiy.storchaka
priority: normal
severity: normal
stage: patch review
status: open
title: Simplify _RandomNameSequence
type: enhancement
versions: Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com