| 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) *  |
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) * |
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) * |
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) * |
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. |
|
|