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

test_pty fails when using setsid() #82728

Closed
vstinner opened this issue Oct 21, 2019 · 12 comments
Closed

test_pty fails when using setsid() #82728

vstinner opened this issue Oct 21, 2019 · 12 comments
Labels
3.9 only security fixes tests Tests in the Lib/test dir

Comments

@vstinner
Copy link
Member

BPO 38547
Nosy @db3l, @vstinner, @pablogsal, @miss-islington
PRs
  • bpo-38547: Fix test_pty if the process is the session leader #17519
  • [3.8] bpo-38547: Fix test_pty if the process is the session leader (GH-17519) #17520
  • [3.7] bpo-38547: Fix test_pty if the process is the session leader (GH-17519) #17521
  • 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 2019-12-09.11:00:12.673>
    created_at = <Date 2019-10-21.11:38:39.966>
    labels = ['tests', '3.9']
    title = 'test_pty fails when using setsid()'
    updated_at = <Date 2019-12-09.11:15:27.344>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2019-12-09.11:15:27.344>
    actor = 'miss-islington'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-12-09.11:00:12.673>
    closer = 'vstinner'
    components = ['Tests']
    creation = <Date 2019-10-21.11:38:39.966>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 38547
    keywords = ['patch']
    message_count = 12.0
    messages = ['355059', '357402', '357403', '357414', '357415', '357416', '357419', '358065', '358066', '358067', '358068', '358070']
    nosy_count = 4.0
    nosy_names = ['db3l', 'vstinner', 'pablogsal', 'miss-islington']
    pr_nums = ['17519', '17520', '17521']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue38547'
    versions = ['Python 3.9']

    @vstinner
    Copy link
    Member Author

    regrtest has been modified in bpo-38502 to use setsid() when using multiprocessing mode (-jN command line option).

    Problem: David Bolen identified that test_pty started to fail on his bolen-ubuntu worker (Ubuntu 18.04.3) since my commit ecb035c.

    https://buildbot.python.org/all/#/builders/141/builds/2679

    0:19:05 load avg: 1.81 [234/419/1] test_pty crashed (Exit code -1) -- running: test_unicodedata (55.5 sec)

    I can reproduce the issue locally:

    ---

    $ ./python -m test -j2 test_pty -v
    == CPython 3.9.0a0 (heads/urlparse_ipv6:cc733a8cb6, Oct 21 2019, 11:34:36) [GCC 9.2.1 20190827 (Red Hat 9.2.1-1)]
    == Linux-5.2.18-200.fc30.x86_64-x86_64-with-glibc2.29 little-endian
    == cwd: /home/vstinner/python/master/build/test_python_20242
    == CPU count: 8
    == encodings: locale=UTF-8, FS=utf-8
    0:00:00 load avg: 0.70 Run tests in parallel using 2 child processes
    0:00:00 load avg: 0.70 [1/1/1] test_pty crashed (Exit code -1)
    test_basic (test.test_pty.PtyTest) ...

    == Tests result: FAILURE ==

    1 test failed:
    test_pty

    Total duration: 383 ms
    Tests result: FAILURE
    ---

    It's surprising that there is no output!

    I would prefer to keep process groups in regrtest, it's really helpful to be able to kill all processes spawned by a test worker process.

    I'm not sure how/why PTY depends is incompatible with setsid().

    @vstinner vstinner added 3.9 only security fixes tests Tests in the Lib/test dir labels Oct 21, 2019
    @pablogsal
    Copy link
    Member

    This also happens when running the test suite with high parallelism:

    ./python -m test -j 20

    This fails with:

    == Tests result: FAILURE ==

    398 tests OK.

    2 tests failed:
    test_embed test_pty

    @pablogsal
    Copy link
    Member

    Indeed, almost all buildbots have to repeat the test_pty.

    I think this needs to be fixed or process groups in regrtest should be limited or reverted.

    @vstinner
    Copy link
    Member Author

    I think this needs to be fixed or process groups in regrtest should be limited or reverted.

    What do you mean by "limited"?

    Process groups really help to prevent to leak grandchild processes in multiprocessing tests, when tests are interrupted manually by CTRL+C or by a timeout (sadly, only when the timeout is handled by regrtest, not when it's handled by faulthandler).

    See bpo-38502 for the rationale.

    @pablogsal
    Copy link
    Member

    What do you mean by "limited"?

    I mean to deactivate it by default and make opt-in.

    Process groups really help to prevent to leak grandchild processes in multiprocessing tests, when tests are interrupted manually by CTRL+C or by a timeout (sadly, only when the timeout is handled by regrtest, not when it's handled by faulthandler).

    I love process groups and they are awesome. But having these test being re-run on every buildbot and failing on my machine when just running test with -j is very annoying.

    @vstinner
    Copy link
    Member Author

    Can't we fix test_pty?

    @db3l
    Copy link
    Contributor

    db3l commented Nov 24, 2019

    I think fixing the underlying pty issue should certainly be the goal, but the question is whether the process group change should remain active in the meantime, as its presence is causing a regression in the tests. I think such cases in the past are usually rolled back, right?

    I was originally on the fence since process groups address a real problem, especially in interactive testing, while creating an arguably aesthetic issue for my case of the buildbots (a warning rather than failure).

    But Pablo's point about a normal manual full test run failing (not a warning as with the buildbots) feels persuasive since that's probably as common as the issue being addressed by the change. Even if pre-existing, the pty failure is exposed by the process group change, so it might be best for the process group change to wait on fixing the pty issue.

    I don't know how to weigh the relative impact though, e.g,. how many people are likely to run into each failure case. There's probably more people doing a normal test run than breaking out of such tests though. At the least, it's a worst impact than just the warnings on the buildbots.

    Perhaps an intermediate fallback could be gating the process group change behind a regrtest option (opt-in) which could then preserve its benefits upon request, without negatively impacting the default test process, whether manual or on the buildbots.

    At least until resource is available to resolve the pty issue.

    @vstinner
    Copy link
    Member Author

    vstinner commented Dec 9, 2019

    I think fixing the underlying pty issue should certainly be the goal (...)

    I wrote PR 17519 which fix the bug. We just have to ignore SIGHUP signal.

    @vstinner
    Copy link
    Member Author

    vstinner commented Dec 9, 2019

    New changeset a1838ec by Victor Stinner in branch 'master':
    bpo-38547: Fix test_pty if the process is the session leader (GH-17519)
    a1838ec

    @vstinner
    Copy link
    Member Author

    vstinner commented Dec 9, 2019

    Ok, I merged my fix to master. The backport to 3.7 and 3.8 will follow quickly. I close the issue.

    Sorry for the inconvenience ;-)

    @vstinner vstinner closed this as completed Dec 9, 2019
    @miss-islington
    Copy link
    Contributor

    New changeset b9f4b49 by Miss Islington (bot) in branch '3.7':
    bpo-38547: Fix test_pty if the process is the session leader (GH-17519)
    b9f4b49

    @miss-islington
    Copy link
    Contributor

    New changeset d08fd29 by Miss Islington (bot) in branch '3.8':
    bpo-38547: Fix test_pty if the process is the session leader (GH-17519)
    d08fd29

    @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
    3.9 only security fixes tests Tests in the Lib/test dir
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants