threading.Semaphore()'s counter can become negative for non-ints
Hello python-dev, This is probably worth of a bug report: While looking at threading.py I noticed that Semaphore's counter can go below zero. This is opposed to the docs: "The counter can never go below zero; ...". Just try: import threading s = threading.Semaphore(0.5) # You can now acquire s as many times as you want! # even when s._value < 0. The fix is tiny: diff -r 265d35e8fe82 Lib/threading.py --- a/Lib/threading.py Fri Jan 27 21:17:04 2012 +0000 +++ b/Lib/threading.py Sat Jan 28 21:22:04 2012 +0200 @@ -322,7 +321,7 @@ rc = False endtime = None self._cond.acquire() - while self._value == 0: + while self._value <= 0: if not blocking: break if __debug__: Which is better than forcing s._value to be an int. I also think that the docs should be updated to reflect that the counter is not compared to be equal to zero, but non-positive. e.g. "when acquire() finds that it is zero...", "If it is zero on entry, block...". On another commit: Regarding http://bugs.python.org/issue9346, an unused import was left: -from collections import deque Cheers, TB
2012/1/28 T.B. <bauertomer@gmail.com>:
Hello python-dev,
This is probably worth of a bug report: While looking at threading.py I noticed that Semaphore's counter can go below zero. This is opposed to the docs: "The counter can never go below zero; ...". Just try:
import threading s = threading.Semaphore(0.5)
But why would you want to pass a float? It seems like API abuse to me. -- Regards, Benjamin
import threading s = threading.Semaphore(0.5)
But why would you want to pass a float? It seems like API abuse to me.
If something should be changed, Semaphore(arg) should raise a TypeError if arg is not an integer. Victor
On 2012-01-30 01:46, Victor Stinner wrote:
But why would you want to pass a float? It seems like API abuse to me.
If something should be changed, Semaphore(arg) should raise a TypeError if arg is not an integer.
Short version: I propose the the change to be - while self._value == 0: + while self._value < 1: This should not change the flow when Semaphore._value is an int. Longer explanation: I thought it is surprising to use math.floor() for threading.Semaphore, but now as you propose, we will need to use something like int(math.floor(value)) in Python2.x - which is even more surprising. That is because math.floor() (and round() for that matter) return a float object in Python2.x. Note: isinstance(4.0, numbers.Integral) is False, even in Python3.x, but until now 4.0 was valid as a value for Semaphore(). Also, using the builtin int()/math.trunc() on a float is probably not what you want here, but rather math.floor(). The value argument given to threading.Semaphore() is really a duck (or an object) that can be compared to 0 and 1, incremented by 1 and decremented by 1. These are properties that fit float. Why should you force the entire builtin int behavior on that object? I agree that using a float as the counter smells bad, but at times you might have something like a fractional resource (which is different from a floating point number). In such cases Semaphore.acquire(), after the tiny patch above, can be thought as checking if you have at least one "unit of resource" available. If you do have at least one such resource - acquire it. This will make sure the invariant "The counter can never go below zero" holds. Regards, TB
TB, what's your use case for passing a float to a semaphore? Semaphores are conceptually tied to integers. You've kept arguing a few times now that the workaround you need are clumsy, but you've not explained why you're passing floats in the first place. A "fractional resource" just doesn't sound like a real use case to me. On Mon, Jan 30, 2012 at 10:40 AM, T.B. <bauertomer@gmail.com> wrote:
On 2012-01-30 01:46, Victor Stinner wrote:
But why would you want to pass a float? It seems like API abuse to me.
If something should be changed, Semaphore(arg) should raise a TypeError if arg is not an integer.
Short version: I propose the the change to be
- while self._value == 0: + while self._value < 1: This should not change the flow when Semaphore._value is an int.
Longer explanation: I thought it is surprising to use math.floor() for threading.Semaphore, but now as you propose, we will need to use something like int(math.floor(value)) in Python2.x - which is even more surprising. That is because math.floor() (and round() for that matter) return a float object in Python2.x.
Note: isinstance(4.0, numbers.Integral) is False, even in Python3.x, but until now 4.0 was valid as a value for Semaphore(). Also, using the builtin int()/math.trunc() on a float is probably not what you want here, but rather math.floor().
The value argument given to threading.Semaphore() is really a duck (or an object) that can be compared to 0 and 1, incremented by 1 and decremented by 1. These are properties that fit float. Why should you force the entire builtin int behavior on that object?
I agree that using a float as the counter smells bad, but at times you might have something like a fractional resource (which is different from a floating point number). In such cases Semaphore.acquire(), after the tiny patch above, can be thought as checking if you have at least one "unit of resource" available. If you do have at least one such resource - acquire it. This will make sure the invariant "The counter can never go below zero" holds.
Regards, TB
_______________________________________________ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/guido%40python.org
-- --Guido van Rossum (python.org/~guido)
On 2012-01-30 20:52, Guido van Rossum wrote:
TB, what's your use case for passing a float to a semaphore? Semaphores are conceptually tied to integers. You've kept arguing a few times now that the workaround you need are clumsy, but you've not explained why you're passing floats in the first place. A "fractional resource" just doesn't sound like a real use case to me.
Not an example from real life and certainly not one that can't be worked around; rather a thing that caught my eyes while looking at Lib/threading.py: Say you have a "known" constant guaranteed bandwidth and you need to split it among several connections which each of them take a known fixed amount of bandwidth (no more, no less). How many connections can I reliably serve? TOTAL_BANDWIDTH/BANDWIDTH_PER_CONNECTION. Well, actually int(_)... Side note: If someone really want a discrete math implementation of a semaphore, you can replace _value with a list of resources. Then you check in acquire() "while not self._resources:" and pop a resource. In that case when a semaphore is used as a context manager it can have a useful 'as' clause. To me it seems too complicated for something that should be simple like a semaphore. Regards, TB
It's also potentially lossy if you incremented and decremented until integer precision is lost. My vote is for an int type check. No casting.
On Mon, Jan 30, 2012 at 2:11 PM, Matt Joiner <anacrolix@gmail.com> wrote:
It's also potentially lossy if you incremented and decremented until integer precision is lost. My vote is for an int type check. No casting.
+1. Anything else is insane scope creep for something called "Semaphore". -- --Guido van Rossum (python.org/~guido)
On Tue, Jan 31, 2012 at 8:11 AM, Matt Joiner <anacrolix@gmail.com> wrote:
It's also potentially lossy if you incremented and decremented until integer precision is lost. My vote is for an int type check. No casting.
operator.index() is built for that purpose (it's what we use these days to restrict slicing to integers). +1 for the type restriction from me. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
2012/1/30 Nick Coghlan <ncoghlan@gmail.com>:
On Tue, Jan 31, 2012 at 8:11 AM, Matt Joiner <anacrolix@gmail.com> wrote:
It's also potentially lossy if you incremented and decremented until integer precision is lost. My vote is for an int type check. No casting.
operator.index() is built for that purpose (it's what we use these days to restrict slicing to integers).
+1 for the type restriction from me.
We don't need a type check. Just pass integers (obviously the only right type) to it. -- Regards, Benjamin
On 2012-01-31 00:23, Benjamin Peterson wrote:
2012/1/30 Nick Coghlan<ncoghlan@gmail.com>:
On Tue, Jan 31, 2012 at 8:11 AM, Matt Joiner<anacrolix@gmail.com> wrote:
It's also potentially lossy if you incremented and decremented until integer precision is lost. My vote is for an int type check. No casting.
operator.index() is built for that purpose (it's what we use these days to restrict slicing to integers).
+1 for the type restriction from me.
We don't need a type check. Just pass integers (obviously the only right type) to it.
When a float is used, think of debugging such a thing, e.g. a float from integer division. I don't care if float (or generally non-integers) are not allowed in threading.Semaphore, but please make it fail with a bang. Regards, TB
On Jan 29, 2012, at 6:11 PM, John O'Connor wrote:
On Sat, Jan 28, 2012 at 3:07 PM, Benjamin Peterson <benjamin@python.org> wrote:
But why would you want to pass a float? It seems like API abuse to me.
Agreed. Anything else seems meaningless.
I concur. This is very much a non-problem. There is no need to add more code and slow running time with superfluous type checks. Raymond
I concur. This is very much a non-problem. There is no need to add more code and slow running time with superfluous type checks.
Raymond
What do you think about the following check from threading.py: @@ -317,8 +317,6 @@ self._value = value def acquire(self, blocking=True, timeout=None): - if not blocking and timeout is not None: - raise ValueError("can't specify timeout for non-blocking acquire") rc = False (There are similar checks in Modules/_threadmodule.c) Removing the check means that we ignore the timeout argument when blocking=False. Currently in the multiprocessing docs there is an outdated note concerning acquire() methods that also says: "If block is False then timeout is ignored". This makes the acquire() methods of the threading and multiprocessing modules have different behaviors. Related: http://bugs.python.org/issue850728#msg103227 TB
On 1/31/2012 3:10 PM, Raymond Hettinger wrote:
On Jan 29, 2012, at 6:11 PM, John O'Connor wrote:
On Sat, Jan 28, 2012 at 3:07 PM, Benjamin Peterson <benjamin@python.org <mailto:benjamin@python.org>> wrote:
But why would you want to pass a float? It seems like API abuse to me.
Agreed. Anything else seems meaningless.
I concur. This is very much a non-problem. There is no need to add more code and slow running time with superfluous type checks.
If it does not now, the doc could be changed to say that the arg must be an int, and behavior is undefined otherwise. Then the contract is clear. -- Terry Jan Reedy
participants (9)
-
Benjamin Peterson -
Guido van Rossum -
John O'Connor -
Matt Joiner -
Nick Coghlan -
Raymond Hettinger -
T.B. -
Terry Reedy -
Victor Stinner