classification
Title: threading.Semaphore does not use try...finally
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.4, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: glglgl, pitrou, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2011-03-29 20:08 by glglgl, last changed 2013-04-22 20:05 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
threading.patch glglgl, 2011-03-29 20:51 patch removing the said issue
threading.patch glglgl, 2011-03-31 21:13 Patch to be applied to 69f58be4688a review
threading.b36cb4602e21.patch glglgl, 2011-03-31 21:20 Patch to be applied to b36cb4602e21
Messages (10)
msg132515 - (view) Author: Thomas Rachel (glglgl) Date: 2011-03-29 20:08
The acquire() and release() functions of threading.Semaphore do not make use of the try...finally suite as it would be reasonable.
They just do 

    def acquire(self, blocking=1):
        rc = False
        self.__cond.acquire()
        [...]
        self.__cond.release()
        return rc

    [...]

    def release(self):
        self.__cond.acquire()
        [...]
        self.__cond.release()

while IMO it would be appropriate to put a try: after the acquire() calls and a finally: before the release() calls so the lock is not held forever if an exception occurs.

(Feel free to use with self.__cond: instead...)

Especially when Ctrl-C is pressed while acquire() waits, the respective KeyboardInterrupt gets thrown after acquire(), breaking the respective function and the __cond is locked forever because it is never release()d.
msg132523 - (view) Author: Thomas Rachel (glglgl) Date: 2011-03-29 20:28
I wonder if it is right what I wrote. After a second thought, the acquire() should come *after* try:, as well in threading.Event. Because if Ctrl-C is pressed while waiting in acquire(), a KeyboardInterrupt is thrown immediately after returning from acquire(). This lock must be release()d again in order to be consistent.
msg132524 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-03-29 20:34
It would be simpler to use "with" indeed.
Do you want to provide a patch?
msg132525 - (view) Author: Thomas Rachel (glglgl) Date: 2011-03-29 20:51
Of course.

I hope it is correct, and I hop it is ok when I change to use "with ...:" on the other places I consider it useful as well...
msg132603 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-03-30 18:59
What version of Python did you make your patch against? It failed applying against the default branch.
You might want to make sure you are using an up-to-date source tree, see: http://docs.python.org/devguide/setup.html#getting-the-source-code
msg132701 - (view) Author: Thomas Rachel (glglgl) Date: 2011-03-31 21:13
Oops, sorry. I have re-made it; it is to be applied to 69f58be4688a.

The original one was made against the respective file of my distribution which contains Python 2.6. It can be applied to (at least) 787b969d37f0, a fact which might help backporting it to 2.x, if wanted.
msg132703 - (view) Author: Thomas Rachel (glglgl) Date: 2011-03-31 21:20
Here is another patch which fits to 2.7, if desired.
msg181547 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-06 18:03
LGTM. 3.x patch a little desynchronized but it is easy to update it. I'll commit it if Antoine have no objections.

Thomas Rachel, can you please submit a contributor form?

http://python.org/psf/contrib/contrib-form/
http://python.org/psf/contrib/
msg187586 - (view) Author: Roundup Robot (python-dev) Date: 2013-04-22 19:57
New changeset 2253b8a18bbf by Serhiy Storchaka in branch '2.7':
Issue #11714: Use 'with' statements to assure a Semaphore releases a
http://hg.python.org/cpython/rev/2253b8a18bbf

New changeset af30c5cb248f by Serhiy Storchaka in branch '3.3':
Issue #11714: Use 'with' statements to assure a Semaphore releases a
http://hg.python.org/cpython/rev/af30c5cb248f

New changeset a26df2d03989 by Serhiy Storchaka in branch 'default':
Issue #11714: Use 'with' statements to assure a Semaphore releases a
http://hg.python.org/cpython/rev/a26df2d03989
msg187588 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-04-22 20:05
Thank you for the patch.

'try/finally' can't be replaced by 'with' statements in the _Event class because a conditional variable should be reinitialized in _reset_internal_locks() after fork.
History
Date User Action Args
2013-04-22 20:05:03serhiy.storchakasetstatus: open -> closed
versions: - Python 3.2
messages: + msg187588

resolution: fixed
stage: commit review -> resolved
2013-04-22 19:57:51python-devsetnosy: + python-dev
messages: + msg187586
2013-02-06 18:03:13serhiy.storchakasetversions: + Python 3.4
nosy: + serhiy.storchaka

messages: + msg181547

assignee: serhiy.storchaka
stage: commit review
2011-03-31 21:20:07glglglsetfiles: + threading.b36cb4602e21.patch

messages: + msg132703
2011-03-31 21:13:12glglglsetfiles: + threading.patch

messages: + msg132701
2011-03-30 18:59:14pitrousetmessages: + msg132603
2011-03-29 20:51:53glglglsetfiles: + threading.patch
type: behavior
messages: + msg132525

keywords: + patch
2011-03-29 20:34:40pitrousetnosy: + pitrou

messages: + msg132524
versions: + Python 3.3, - Python 2.6, Python 3.1
2011-03-29 20:28:21glglglsetmessages: + msg132523
2011-03-29 20:08:05glglglcreate