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_inspect fails in refleak mode #88971

Closed
pablogsal opened this issue Aug 2, 2021 · 16 comments
Closed

test_inspect fails in refleak mode #88971

pablogsal opened this issue Aug 2, 2021 · 16 comments
Labels
3.10 only security fixes 3.11 only security fixes

Comments

@pablogsal
Copy link
Member

BPO 44808
Nosy @ambv, @markshannon, @pablogsal, @miss-islington, @akulakov
PRs
  • bpo-44808: Fix test_inspect in refleak mode #27544
  • [3.10] bpo-44808: Fix test_inspect in refleak mode (GH-27544) #27546
  • bpo-44808: fixes test for interactive inspect getsource of a class #27571
  • [3.10] bpo-44808: fixes test for interactive inspect getsource of a class (GH-27571) #27578
  • 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 2021-08-03.13:08:55.600>
    created_at = <Date 2021-08-02.09:55:50.687>
    labels = ['3.10', '3.11']
    title = 'test_inspect fails in refleak mode'
    updated_at = <Date 2021-08-03.13:08:55.600>
    user = 'https://github.com/pablogsal'

    bugs.python.org fields:

    activity = <Date 2021-08-03.13:08:55.600>
    actor = 'lukasz.langa'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-08-03.13:08:55.600>
    closer = 'lukasz.langa'
    components = []
    creation = <Date 2021-08-02.09:55:50.687>
    creator = 'pablogsal'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 44808
    keywords = ['patch']
    message_count = 16.0
    messages = ['398744', '398745', '398747', '398749', '398750', '398752', '398753', '398754', '398755', '398757', '398759', '398767', '398788', '398812', '398824', '398825']
    nosy_count = 5.0
    nosy_names = ['lukasz.langa', 'Mark.Shannon', 'pablogsal', 'miss-islington', 'andrei.avk']
    pr_nums = ['27544', '27546', '27571', '27578']
    priority = None
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue44808'
    versions = ['Python 3.10', 'Python 3.11']

    @pablogsal
    Copy link
    Member Author

    From https://bugs.python.org/issue44206 :

    File "/home/mark/repos/cpython/Lib/inspect.py", line 1154, in walktree
    classes.sort(key=attrgetter('__module__', '__name__'))
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    TypeError: '<' not supported between instances of 'str' and 'module'

    I can reproduce that failure with a debug build and either of
    ./python -m test -R 3:3 test_inspect
    ./python -m test test_inspect -F

    @pablogsal pablogsal added 3.10 only security fixes 3.11 only security fixes labels Aug 2, 2021
    @pablogsal
    Copy link
    Member Author

    I'm bisecting

    @pablogsal
    Copy link
    Member Author

    f468ede is the first bad commit
    commit f468ede
    Author: Miss Islington (bot) <31488909+miss-islington@users.noreply.github.com>
    Date: Fri Jul 30 10:46:42 2021 -0700

    bpo-44648: Fix error type in inspect.getsource() in interactive session (GH-27171) (GH-27495)
    
    (cherry picked from commit 48a62559dfaf775e4f1cc56b19379c799e8e2587)
    
    Co-authored-by: andrei kulakov <andrei.avk@gmail.com>
    

    Lib/inspect.py | 2 ++
    Lib/test/test_inspect.py | 17 ++++++++++++++++-
    .../Library/2021-07-15-16-51-32.bpo-44648.2o49TB.rst | 3 +++
    3 files changed, 21 insertions(+), 1 deletion(-)
    create mode 100644 Misc/NEWS.d/next/Library/2021-07-15-16-51-32.bpo-44648.2o49TB.rst

    @pablogsal
    Copy link
    Member Author

    The reason buildbots have not catched this is because the re-run login seems to not detect the problem on the second run:

    ./python.exe -m test test_inspect -j 1 -u all -W --slowest --fail-env-changed --timeout=11700 -R 3:3 -u-cpu -w

    WARNING: Disable --verbose3 because it's incompatible with --huntrleaks: see http://bugs.python.org/issue27103
    0:00:00 load avg: 2.73 Run tests in parallel using 1 child processes (timeout: 3 hour 15 min, worker timeout: 3 hour 20 min)
    0:00:03 load avg: 2.92 [1/1/1] test_inspect failed (1 error)
    beginning 6 repetitions
    123456
    .test test_inspect failed -- Traceback (most recent call last):
      File "/Users/pgalindo3/github/cpython/Lib/test/test_inspect.py", line 398, in test_getclasses
        tree = inspect.getclasstree([cls[1] for cls in classes])
      File "/Users/pgalindo3/github/cpython/Lib/inspect.py", line 1185, in getclasstree
        return walktree(roots, children, None)
      File "/Users/pgalindo3/github/cpython/Lib/inspect.py", line 1158, in walktree
        results.append(walktree(children[c], children, c))
      File "/Users/pgalindo3/github/cpython/Lib/inspect.py", line 1154, in walktree
        classes.sort(key=attrgetter('__module__', '__name__'))
    TypeError: '<' not supported between instances of 'str' and 'module'

    == Tests result: FAILURE ==

    10 slowest tests:

    • test_inspect: 3.4 sec

    1 test failed:
    test_inspect

    1 re-run test:
    test_inspect

    0:00:03 load avg: 2.92 Re-running failed tests in verbose mode
    0:00:03 load avg: 2.92 Re-running test_inspect in verbose mode (matching: test_getclasses)
    beginning 6 repetitions
    123456
    test_getclasses (test.test_inspect.TestRetrievingSourceCode) ... ok

    ----------------------------------------------------------------------

    Ran 1 test in 0.001s

    OK
    .test_getclasses (test.test_inspect.TestRetrievingSourceCode) ... ok

    ----------------------------------------------------------------------

    Ran 1 test in 0.001s

    OK
    .test_getclasses (test.test_inspect.TestRetrievingSourceCode) ... ok

    ----------------------------------------------------------------------

    Ran 1 test in 0.001s

    OK
    .test_getclasses (test.test_inspect.TestRetrievingSourceCode) ... ok

    ----------------------------------------------------------------------

    Ran 1 test in 0.001s

    OK
    .test_getclasses (test.test_inspect.TestRetrievingSourceCode) ... ok

    ----------------------------------------------------------------------

    Ran 1 test in 0.001s

    OK
    .test_getclasses (test.test_inspect.TestRetrievingSourceCode) ... ok

    ----------------------------------------------------------------------

    Ran 1 test in 0.001s

    OK
    .

    == Tests result: FAILURE then SUCCESS ==

    1 test OK.

    10 slowest tests:

    • test_inspect: 3.4 sec

    Total duration: 4.6 sec
    Tests result: FAILURE then SUCCESS

    @pablogsal
    Copy link
    Member Author

    @pablogsal
    Copy link
    Member Author

    From f468ede:

    This code looks quite dangerous:

    class TestGetsourceInteractive(unittest.TestCase):
        def tearDown(self):
            mod.ParrotDroppings.__module__ = mod
            sys.modules['__main__'] = self.main

    That is modifing sys.modules globally!

    @pablogsal
    Copy link
    Member Author

    Ah, the problem is that:

            mod.ParrotDroppings.__module__ = mod

    is wrong: __module__ must be a string, not a module object

    @pablogsal
    Copy link
    Member Author

    PR 27544 just addresses the main problem, but I still have concerns about the test affecting sys.modules directly, making it dangerous to run in parallel.

    @pablogsal
    Copy link
    Member Author

    With PR 27544:

    ❯ ./python.exe -m test test_inspect -R :
    0:00:00 load avg: 2.91 Run tests sequentially
    0:00:00 load avg: 2.91 [1/1] test_inspect
    beginning 9 repetitions
    123456789
    .........

    == Tests result: SUCCESS ==

    1 test OK.

    Total duration: 13.7 sec
    Tests result: SUCCESS

    @ambv
    Copy link
    Contributor

    ambv commented Aug 2, 2021

    New changeset 626d397 by Pablo Galindo Salgado in branch 'main':
    bpo-44808: Fix test_inspect in refleak mode (GH-27544)
    626d397

    @akulakov
    Copy link
    Contributor

    akulakov commented Aug 2, 2021

    Pablo: thanks for investigating and fixing this, I will look for examples of changing or patching sys.modules in the testsuite to see how to avoid global effects. I think we can keep this issue open for the follow-up discussion / fix?

    @miss-islington
    Copy link
    Contributor

    New changeset a1eaa74 by Miss Islington (bot) in branch '3.10':
    bpo-44808: Fix test_inspect in refleak mode (GH-27544)
    a1eaa74

    @akulakov
    Copy link
    Contributor

    akulakov commented Aug 2, 2021

    I've been looking into avoiding global modification of modules['__main__'], the options are:

    1. remove the test and leave it at that

    2. remove the test and open a new issue for re-adding a test ; and look for a way to do it safely

    3. create a temp script file, add code to it that creates a class, sets sys.modules['__main__'].__file__ = None and inspects the class, and run this module in a new interpreter, checks exception raised and does sys.exit(1); and check the exit code after running this script.

    4. patch inspect.sys.modules; as far as I understand, this would mean other modules accessing sys.modules in other threads won't be affected, and sys.modules['__main__'] is currently used in inspect in a very limited way, so it can probably be shown that this patching won't affect other tests that use inspect, -- but I would need to look more into this.

    I'm not sure if #3 is too heavy/complex for this test.

    The advantage of #1 and #2 is that it's easy to do and ensures that there won't be strange random test failures that are hard to track down and debug (which is something that I'm afraid can happen right now, caused by this test).

    #4 I've added to get comments / thought.

    @akulakov
    Copy link
    Contributor

    akulakov commented Aug 3, 2021

    Please disregard my last comment, I think I found a good way to fix it, the PR is up here:
    https://github.com/python/cpython/pull/27571/files

    @ambv
    Copy link
    Contributor

    ambv commented Aug 3, 2021

    New changeset 5832597 by andrei kulakov in branch 'main':
    bpo-44808: fixes test for interactive inspect getsource of a class (GH-27571)
    5832597

    @miss-islington
    Copy link
    Contributor

    New changeset bc2841c by Miss Islington (bot) in branch '3.10':
    bpo-44808: fixes test for interactive inspect getsource of a class (GH-27571)
    bc2841c

    @ambv ambv closed this as completed Aug 3, 2021
    @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.10 only security fixes 3.11 only security fixes
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants