classification
Title: Remove .stopped Event from Thread internals
Type: resource usage Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: 18808 Superseder:
Assigned To: tim.peters Nosy List: pitrou, python-dev, tim.peters
Priority: normal Keywords: needs review, patch

Created on 2013-09-08 21:46 by tim.peters, last changed 2013-09-09 20:09 by tim.peters. This issue is now closed.

Files
File name Uploaded Description Edit
remove_stopped tim.peters, 2013-09-08 21:46 remove .stopped from Thread review
remove_stopped_2.patch pitrou, 2013-09-08 22:14 review
cleanup.patch tim.peters, 2013-09-09 19:15 review
cleanup2.patch pitrou, 2013-09-09 19:31 review
Messages (12)
msg197325 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2013-09-08 21:46
As discussed in issue 18808, now that we're checking for a tstate lock, the Thread._stopped Event has become an "attractive nuisance".  The attached patch removes it.

This simplifies .join() and .is_alive(), and restores pre-18808 .join(timeout) endcase behavior (i.e., if join returns before the timeout expires, .is_alive() will always be False).

Since this doesn't add any new locks, I hope it won't create more problems with fork tests - but running on Windows I wouldn't know ;-)
msg197326 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-08 21:50
Hmm.. I'm getting the following failures:

======================================================================
FAIL: test_4_joining_across_fork_in_worker_thread (test.test_threading.ThreadJoinOnShutdown)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/antoine/cpython/default/Lib/test/test_threading.py", line 726, in test_4_joining_across_fork_in_worker_thread
    self.assertScriptHasOutput(script, "end of main\n")
  File "/home/antoine/cpython/default/Lib/test/test_threading.py", line 651, in assertScriptHasOutput
    rc, out, err = assert_python_ok("-c", script)
  File "/home/antoine/cpython/default/Lib/test/script_helper.py", line 55, in assert_python_ok
    return _assert_python(True, *args, **env_vars)
  File "/home/antoine/cpython/default/Lib/test/script_helper.py", line 46, in _assert_python
    "stderr follows:\n%s" % (rc, err.decode('ascii', 'ignore')))
AssertionError: Process return code is 1, stderr follows:
Traceback (most recent call last):
  File "<string>", line 29, in <module>
AttributeError: 'Thread' object has no attribute '_stopped'

======================================================================
FAIL: test_5_clear_waiter_locks_to_avoid_crash (test.test_threading.ThreadJoinOnShutdown)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/antoine/cpython/default/Lib/test/test_threading.py", line 784, in test_5_clear_waiter_locks_to_avoid_crash
    self.assertScriptHasOutput(script, output)
  File "/home/antoine/cpython/default/Lib/test/test_threading.py", line 651, in assertScriptHasOutput
    rc, out, err = assert_python_ok("-c", script)
  File "/home/antoine/cpython/default/Lib/test/script_helper.py", line 55, in assert_python_ok
    return _assert_python(True, *args, **env_vars)
  File "/home/antoine/cpython/default/Lib/test/script_helper.py", line 46, in _assert_python
    "stderr follows:\n%s" % (rc, err.decode('ascii', 'ignore')))
AssertionError: Process return code is 1, stderr follows:
Traceback (most recent call last):
  File "<string>", line 29, in <module>
AttributeError: 'Thread' object has no attribute '_stopped'
msg197327 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-08 21:52
Looks like it's mostly the tests trying to check an implementation detail that doesn't exist anymore :-)
msg197328 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2013-09-08 22:01
I agree with your diagnosis.  Unfortunately, I can't test the fork stuff.  Well, OK, I actually think that's fortunate (for me ;-) ).

If you can see a quick way to fix these, please do.  I'm going to vanish for about 2 hours now.
msg197330 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-08 22:14
I think the fix is simply to remove the outdated tests.

I've been running test_threading in a loop with the (attached) updated patch, and there hasn't been a single failure.
msg197337 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-09-08 23:46
New changeset aff959a3ba13 by Tim Peters in branch 'default':
Issue 18984:  Remove ._stopped Event from Thread internals.
http://hg.python.org/cpython/rev/aff959a3ba13
msg197341 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2013-09-08 23:50
Thanks, Antoine!  I pushed this change, figuring that even if some buildbots are still unhappy about 18808, getting rid of the Event can't make them more unhappy, and may make them happier.
msg197378 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2013-09-09 19:15
Antoine, could I bother you to try the attached cleanup.patch?  It looks harmless to me, but when I checked it in the Unix-y buildbots failed the thread+fork tests again :-(  Two examples:

http://buildbot.python.org/all/builders/x86%20Gentoo%203.x/builds/4914/steps/test/logs/stdio

http://buildbot.python.org/all/builders/x86%20Ubuntu%20Shared%203.x/builds/8581/steps/test/logs/stdio

Worse, the buildbot slaves appeared never to finish running then - I had to stop them (via the "stop build" button).

Maybe I'm just tired, but I've stared & stared at this and just don't see what's going wrong :-(
msg197382 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-09 19:31
Here is a fixed patch (works here, at least :-)).
msg197386 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2013-09-09 19:37
Well - I remain baffled, but am grateful for the patch - thanks :-)
msg197394 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-09 20:06
I suppose we can close this issue now?
msg197395 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2013-09-09 20:09
Yes - and I just closed 18808 :-)
History
Date User Action Args
2013-09-09 20:09:39tim.peterssetstatus: open -> closed

messages: + msg197395
stage: patch review -> resolved
2013-09-09 20:06:43pitrousetmessages: + msg197394
2013-09-09 19:37:53tim.peterssetmessages: + msg197386
2013-09-09 19:31:53pitrousetfiles: + cleanup2.patch

messages: + msg197382
2013-09-09 19:15:12tim.peterssetfiles: + cleanup.patch

messages: + msg197378
2013-09-08 23:54:04tim.peterssetmessages: - msg197342
2013-09-08 23:53:36tim.peterssetmessages: + msg197342
2013-09-08 23:50:24tim.peterssetresolution: fixed
messages: + msg197341
2013-09-08 23:46:43python-devsetnosy: + python-dev
messages: + msg197337
2013-09-08 22:14:06pitrousetfiles: + remove_stopped_2.patch
type: resource usage
messages: + msg197330
2013-09-08 22:01:21tim.peterssetmessages: + msg197328
2013-09-08 21:52:35pitrousetmessages: + msg197327
2013-09-08 21:50:11pitrousetmessages: + msg197326
2013-09-08 21:47:44tim.peterssetdependencies: + Thread.join returns before PyThreadState is destroyed
2013-09-08 21:46:26tim.peterscreate