Repository navigation
os.urandom() fails under high load #62956
Description
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 2013-08-30.22:27:37.255>
created_at = <Date 2013-08-16.15:21:38.028>
labels = ['extension-modules', 'performance']
title = 'os.urandom() fails under high load'
updated_at = <Date 2013-09-20.12:32:44.373>
user = 'https://github.com/tiran'bugs.python.org fields:
activity = <Date 2013-09-20.12:32:44.373>
actor = 'Adam.Biela\xc5\x84ski'
assignee = 'none'
closed = True
closed_date = <Date 2013-08-30.22:27:37.255>
closer = 'pitrou'
components = ['Extension Modules']
creation = <Date 2013-08-16.15:21:38.028>
creator = 'christian.heimes'
dependencies = []
files = ['31317', '31318', '31319', '31323', '31327', '31449']
hgrepos = []
issue_num = 18756
keywords = ['patch']
message_count = 47.0
messages = ['195338', '195339', '195341', '195345', '195348', '195349', '195350', '195351', '195352', '195353', '195354', '195356', '195357', '195358', '195360', '195361', '195362', '195363', '195364', '195365', '195366', '195367', '195368', '195371', '195372', '195373', '195375', '195381', '195382', '195393', '195394', '195395', '195396', '195398', '195414', '195416', '195450', '195453', '196017', '196083', '196084', '196086', '196087', '196094', '196095', '196589', '196592']
nosy_count = 11.0
nosy_names = ['jcea', 'pitrou', 'vstinner', 'christian.heimes', 'tarek', 'alex', 'neologix', 'Adam.Biela\xc5\x84ski', 'python-dev', 'hynek', 'dstufft']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue18756'
versions = ['Python 3.4']
Activity
tiran commentedon Aug 16, 2013
I have seen complains from e.g. Tarek that os.urandom() fails under high load: https://twitter.com/tarek_ziade/status/362281268215418880
The problem is caused by file descriptor limits. os.urandom() opens /dev/urandom for every call. How about os.urandom() uses a persistent file descriptor? That should eliminate the error. It may alsos speed up os.urandom() because a persistent FD gets rid of open() and close() syscalls.
vstinner commentedon Aug 16, 2013
dev_urandom_python() should handle ENFILE and ENOENT differently to raise a different exception. Or it should always call PyErr_SetFromErrno(PyExc_OSError); ?
Can tarek tell us more about its usecases: is he directly calling os.urandom() or does he use the random module? How many threads?
os.urandom() is called at Python startup to generate a "secret key" for random hash. If the file descriptor is never closed, the next file descriptor will be 4 instead of the expect 3.
Always keeping an internal file descriptor open may have unexpected effects like leaking a file descriptor to a child process... (see the PEP-446, not implemented yet).
I'm ok to keep a fd open if the user controls the lifetime of the object (lifetime of the fd). For example, I expect that rng = SystemRandom() opens /dev/urandom when the object is created, and closes it when the object is destroyed.
pitrou commentedon Aug 16, 2013
I don't think that's bug in os.urandom(). If os.urandom() doesn't fail, something else will fail soon after.
OTOH, the error is clearly misleading. The NotImplementedError should only be raised for certain errnos (such as ENOENT, ENODEV, ENXIO and EACCES), not all of them.
jcea commentedon Aug 16, 2013
I agree with Antoine. Exhausting the FDs is not the problem, the problem is the misleading error.
tarekziade commentedon Aug 16, 2013
the random pool can be exhausted, but this is not "soon after" I think. In Linux and Mac OS X, ulimit -n defaults to 512 and 256.
It's very easy to reach that limit if you write a web app that uses this API.
Do you suggest that we should not use os.urandom on high load ?
Opening an FD on every call sounds under optimal, I am not seeing any drawback not to try to optimize that API.
tarekziade commentedon Aug 16, 2013
I was using ws4py inside greenlets. ws4py uses os.urandom() to generate some keys. So one single thread, many greenlets.
neologix commentedon Aug 16, 2013
I don't think he's referring to the entropy pool, but to RLIMIT_NOFILE.
You'll likely hit EMFILE sooner or later, e.g. on socket(), open()...
What does high load mean?
If you mean many concurrent threads, then you should probably go for
the random module, no?
Well, first we'll have to make the code thread-safe, if we want to
keep a persistent FD open. Which means we'll have to add a lock, which
is likely to reduce concurrency, and overall throughput.
tiran commentedon Aug 16, 2013
It's highly unlikely that you are every going to exhaust the CPRNG to a
point were it is no longer cryptographically secure. Thomas Ptacek
pointed me to http://security.stackexchange.com/a/3939 yesterday.
The drawback is a slightly more complicated implementation that has to
deal with invalid FDs.
tarekziade commentedon Aug 16, 2013
a web app with a few hundreds concurrent requests.
I use greenlets. But, I don't know - are you suggesting os.urandom() should be marked in the documentation as "DOES NOT SCALE" and I should use another API ? Which one ?
neologix commentedon Aug 16, 2013
2013/8/16, Tarek Ziadé <report@bugs.python.org>:
Well, even with greenlets, I assume you're using at least one FD
(socket) per client, no?
So you can get EMFILE on socket() just as on os.urandom(). The only
difference is that sockets are long-lived, whereas os.urandom() only
opens a FD for a couple ms. So os.urandom() isn't your biggest problem
here.
I'd suggest you to just open '/dev/urandom' once, and then make all
your threads/green-threads read from it.
IMO os.urandom() is a really poor API ;-)
tarekziade commentedon Aug 16, 2013
I do many calls on urandom() so that's the FD bottleneck.
Of course it is. But it looks like you know better without having looked at the code. :)
Let me know how to do this without being able to prevent the API to close the FD everytime.
Then we should improve it or deprecate it.
tiran commentedon Aug 16, 2013
Am 16.08.2013 18:24, schrieb Charles-François Natali:
Why locking? /dev/urandom is a pseudo char device. You can have multiple
readers on the same fd without any locking. Did you know that Java keeps
one persistent fd to /dev/urandom?
pitrou commentedon Aug 16, 2013
Unless you're doing many calls *in parallel* it's unlikely to be a
bottleneck.
At worse you can write your own /dev/urandom reading code, with a shared
fd amongst all your threads / greenlets.
os.urandom() is a convenience function, it doesn't have to be extremely
optimized.
23 remaining items
pitrou commentedon Aug 16, 2013
Updated patch after Christian's comments.
tiran commentedon Aug 16, 2013
LGTM
vstinner commentedon Aug 17, 2013
Tarek: try to use ssl.RAND_bytes(), it is secure, fast and don't use a file
descriptor.
IMO if something can be improved, it is in the random.SystemRandom() class:
it can keep the FD open. Does the class have a method to generate random
bytes?
dstufft commentedon Aug 17, 2013
haypo: It's been suggested by a number of security professionals that using the OpenSSL random (or really any random) instead of urandom is likely to be a smarter idea. The likelyhood that urandom is broken is far less than any other source of random. This can be seen in the recent issues on the Android platform. This is not to say that there's a reason to believe that OpenSSL is broken currently, but that the chances are higher for it to be than /dev/urandom. An example of when this happened was http://www.debian.org/security/2008/dsa-1571.
There's no reason to believe that OpenSSL is wrong right now, but the chances of OpenSSL being wrong are greater than the chances of /dev/urandom being
There's been a few threads on twitter about it in light of the Android SecureRandom issue (don't need to read these, just here for reference):
- https://twitter.com/tqbf/status/368089082800246784
- https://twitter.com/tqbf/status/367793231808843777
- https://twitter.com/tqbf/status/368089362333827072
I don't think it actually matters if os.urandom or random.SystemRandom is the preferred interface that keeps the FD open but I do believe there should be one implementation that will use the OS source of random and maintain a persistent FD.
pitrou commentedon Aug 23, 2013
Ok, you're gonna laugh, the simplified patch has a complication (not theoretical, it would trip test_cmd_line). Attaching patch.
neologix commentedon Aug 24, 2013
Antoine, this changeset broke Tiger buildbots badly:
"""
======================================================================
ERROR: test_urandom_failure (test.test_os.URandomTests)
----------------------------------------------------------------------
http://buildbot.python.org/all/builders/x86%20Tiger%203.x/builds/6826/steps/test/logs/stdio
"""
1030 # We restore the old limit as soon as possible. If doing it
1031 # using addCleanup(), code running in between would fail
1032 # creating any file descriptor.
1033 resource.setrlimit(resource.RLIMIT_NOFILE, (soft_limit, hard_limit))
"""
The code trying to reset RLIMIT_NOFILE to its previous value fails,
and as a consequence, all subsequent FDs creation fail (since the soft
limit it 1)...
It looks like Tiger getrlimit() return a nonsensical value (maybe -1),
which means that you can't do setrlimit(getrlimit()), or yet another
OS X bug (TM).
See e.g. http://www.couchbase.com/issues/browse/MB-3064
I'd suggest two things:
would be fine), to be more robust against this
neologix commentedon Aug 24, 2013
Or more precisely, just run the test in a subprocess. That should fix
the OS X failure if we don't restore the RLIMIT_NOFILE limits, and
will make the test more robust (but you can't reuse the new test,
since it won't work with lazy-opening).
python-dev commentedon Aug 24, 2013
New changeset b9e62929460e by Antoine Pitrou in branch '3.3':
Issue bpo-18756: make test_urandom_failure more robust by executing its code in a subprocess
http://hg.python.org/cpython/rev/b9e62929460e
New changeset 68ff013b194c by Antoine Pitrou in branch 'default':
Issue bpo-18756: make test_urandom_failure more robust by executing its code in a subprocess
http://hg.python.org/cpython/rev/68ff013b194c
New changeset 869df611c138 by Antoine Pitrou in branch '2.7':
Issue bpo-18756: make test_urandom_failure more robust by executing its code in a subprocess
http://hg.python.org/cpython/rev/869df611c138
pitrou commentedon Aug 24, 2013
Ok, the tiger should feel better now :-)
pitrou commentedon Aug 24, 2013
So, to come back to the original topic, is everyone sold on the idea of caching the urandom fd lazily?
dstufft commentedon Aug 24, 2013
Lazily opening urandom and holding it open sounds like a sane thing to do to me +1
python-dev commentedon Aug 30, 2013
New changeset acc7439b1406 by Antoine Pitrou in branch 'default':
Issue bpo-18756: os.urandom() now uses a lazily-opened persistent file descriptor, so as to avoid using many file descriptors when run in parallel from multiple threads.
http://hg.python.org/cpython/rev/acc7439b1406
pitrou commentedon Aug 30, 2013
Ok, I've committed the patch for the lazy opening approach.