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

Avoid test_main() in test_httplib; gc.collect() dangling threads #71974

Closed
vadmium opened this issue Aug 17, 2016 · 7 comments
Closed

Avoid test_main() in test_httplib; gc.collect() dangling threads #71974

vadmium opened this issue Aug 17, 2016 · 7 comments
Labels
tests Tests in the Lib/test dir type-feature A feature request or enhancement

Comments

@vadmium
Copy link
Member

vadmium commented Aug 17, 2016

BPO 27787
Nosy @vstinner, @vadmium
Files
  • httplib-tests.patch
  • httplib-tests.v2.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 = None
    closed_at = <Date 2016-08-23.13:10:17.082>
    created_at = <Date 2016-08-17.14:33:36.533>
    labels = ['type-feature', 'tests']
    title = 'Avoid test_main() in test_httplib; gc.collect() dangling threads'
    updated_at = <Date 2016-08-23.18:20:55.818>
    user = 'https://github.com/vadmium'

    bugs.python.org fields:

    activity = <Date 2016-08-23.18:20:55.818>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-08-23.13:10:17.082>
    closer = 'martin.panter'
    components = ['Tests']
    creation = <Date 2016-08-17.14:33:36.533>
    creator = 'martin.panter'
    dependencies = []
    files = ['44134', '44163']
    hgrepos = []
    issue_num = 27787
    keywords = ['patch']
    message_count = 7.0
    messages = ['272954', '272955', '273004', '273193', '273197', '273433', '273502']
    nosy_count = 3.0
    nosy_names = ['vstinner', 'python-dev', 'martin.panter']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue27787'
    versions = ['Python 3.5', 'Python 3.6']

    @vadmium
    Copy link
    Member Author

    vadmium commented Aug 17, 2016

    In bpo-12319, there are many iterations of a patch that adds a new TestCase subclass to Lib/test/test_httplib.py. However it never got run by the main regrtest infrastructure, because nobody remembered to add the class to the list of test classes. So I want to remove test_main(). It seems this would let the classes be automatically discovered (like normal unittest usage).

    I understand @reap_threads is to avoid background threads continuing between tests (especially when a test fails). I improved the cleanup of the background thread in one test. There are three other test methods using test.ssl_servers.make_https_server(), which also runs a background thread, but that already seems to clean itself up properly, via case.addCleanup(server.join).

    I found that the test infrastructure randomly complained about dangling threads without @reap_threads. It uses a set of weak references to thread objects. The solution seems to be to call gc.collect() before checking. This is what @reap_threads does, so maybe my patch would eliminate the need for @reap_threads in other test files as well. In fact, if everybody called join() on their threads, we may be able to eliminate @reap_threads altogether.

    @vadmium vadmium added tests Tests in the Lib/test dir type-feature A feature request or enhancement labels Aug 17, 2016
    @vstinner
    Copy link
    Member

    Your patch changes 3 different things. After the code review, I suggest to split it in two changes (fix test_httplib threading ripper, fix save_env, fix test_httplib main).

    @vadmium
    Copy link
    Member Author

    vadmium commented Aug 18, 2016

    Yes I agree it would make sense to separate the test_httplib changes from the general change. I thought this task would be a very easy change, and noticed it wasn’t that simple the last minute.

    I would like to adjust the cleanup call to

    self.addCleanup(thread.join, float(1))

    That should be a better equivalent to what @reap_threads does. Or I can just use @reap_threads directly on test_response_fileno(), if people prefer.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 20, 2016

    New changeset 7ddbc2263943 by Martin Panter in branch '3.5':
    Issue bpo-27787: Clean up weak references before checking for dangling threads
    https://hg.python.org/cpython/rev/7ddbc2263943

    New changeset e5777c5d108c by Martin Panter in branch 'default':
    Issue bpo-27787: Merge regrtest fixup from 3.5
    https://hg.python.org/cpython/rev/e5777c5d108c

    @vadmium
    Copy link
    Member Author

    vadmium commented Aug 20, 2016

    I committed the gc_collect() change to 3.5 as well, because it helped me with a separate test case.

    Here is the remaining change, for 3.6 only. I added a timeout to the join() call. This matches the @reap_threads decorator, which times out after 1 s.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 23, 2016

    New changeset 1f73355afebb by Martin Panter in branch 'default':
    Issue bpo-27787: Remove test_main() and hard-coded list of test classes
    https://hg.python.org/cpython/rev/1f73355afebb

    @vadmium vadmium closed this as completed Aug 23, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 23, 2016

    New changeset 31e4495a34ce by Terry Jan Reedy in branch 'default':
    Issue bpo-27787: No longer call deleted test_main().
    https://hg.python.org/cpython/rev/31e4495a34ce

    @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
    tests Tests in the Lib/test dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants