Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove .stopped Event from Thread internals #63184

Closed
tim-one opened this issue Sep 8, 2013 · 12 comments
Closed

Remove .stopped Event from Thread internals #63184

tim-one opened this issue Sep 8, 2013 · 12 comments
Assignees
Labels
performance Performance or resource usage stdlib Python modules in the Lib dir

Comments

@tim-one
Copy link
Member

tim-one commented Sep 8, 2013

BPO 18984
Nosy @tim-one, @pitrou
Dependencies
  • bpo-18808: Thread.join returns before PyThreadState is destroyed
  • Files
  • remove_stopped: remove .stopped from Thread
  • remove_stopped_2.patch
  • cleanup.patch
  • cleanup2.patch
  • 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 = 'https://github.com/tim-one'
    closed_at = <Date 2013-09-09.20:09:39.577>
    created_at = <Date 2013-09-08.21:46:26.426>
    labels = ['library', 'performance']
    title = 'Remove .stopped Event from Thread internals'
    updated_at = <Date 2013-09-09.20:09:39.558>
    user = 'https://github.com/tim-one'

    bugs.python.org fields:

    activity = <Date 2013-09-09.20:09:39.558>
    actor = 'tim.peters'
    assignee = 'tim.peters'
    closed = True
    closed_date = <Date 2013-09-09.20:09:39.577>
    closer = 'tim.peters'
    components = ['Library (Lib)']
    creation = <Date 2013-09-08.21:46:26.426>
    creator = 'tim.peters'
    dependencies = ['18808']
    files = ['31690', '31691', '31700', '31701']
    hgrepos = []
    issue_num = 18984
    keywords = ['patch', 'needs review']
    message_count = 12.0
    messages = ['197325', '197326', '197327', '197328', '197330', '197337', '197341', '197378', '197382', '197386', '197394', '197395']
    nosy_count = 3.0
    nosy_names = ['tim.peters', 'pitrou', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'resource usage'
    url = 'https://bugs.python.org/issue18984'
    versions = ['Python 3.4']

    @tim-one
    Copy link
    Member Author

    tim-one commented Sep 8, 2013

    As discussed in bpo-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 ;-)

    @tim-one tim-one self-assigned this Sep 8, 2013
    @tim-one tim-one added the stdlib Python modules in the Lib dir label Sep 8, 2013
    @pitrou
    Copy link
    Member

    pitrou commented Sep 8, 2013

    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'

    @pitrou
    Copy link
    Member

    pitrou commented Sep 8, 2013

    Looks like it's mostly the tests trying to check an implementation detail that doesn't exist anymore :-)

    @tim-one
    Copy link
    Member Author

    tim-one commented Sep 8, 2013

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Sep 8, 2013

    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.

    @pitrou pitrou added the performance Performance or resource usage label Sep 8, 2013
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 8, 2013

    New changeset aff959a3ba13 by Tim Peters in branch 'default':
    bpo-18984: Remove ._stopped Event from Thread internals.
    http://hg.python.org/cpython/rev/aff959a3ba13

    @tim-one
    Copy link
    Member Author

    tim-one commented Sep 8, 2013

    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.

    @tim-one
    Copy link
    Member Author

    tim-one commented Sep 9, 2013

    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 :-(

    @pitrou
    Copy link
    Member

    pitrou commented Sep 9, 2013

    Here is a fixed patch (works here, at least :-)).

    @tim-one
    Copy link
    Member Author

    tim-one commented Sep 9, 2013

    Well - I remain baffled, but am grateful for the patch - thanks :-)

    @pitrou
    Copy link
    Member

    pitrou commented Sep 9, 2013

    I suppose we can close this issue now?

    @tim-one
    Copy link
    Member Author

    tim-one commented Sep 9, 2013

    Yes - and I just closed 18808 :-)

    @tim-one tim-one closed this as completed Sep 9, 2013
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    performance Performance or resource usage stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants