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

inspect.getmodule fails when module imports change sys.modules #57696

Closed
ErikTollerud mannequin opened this issue Nov 27, 2011 · 20 comments
Closed

inspect.getmodule fails when module imports change sys.modules #57696

ErikTollerud mannequin opened this issue Nov 27, 2011 · 20 comments
Assignees
Labels
3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@ErikTollerud
Copy link
Mannequin

ErikTollerud mannequin commented Nov 27, 2011

BPO 13487
Nosy @gpshead, @amauryfa, @ned-deily, @merwok, @meadori, @ericsnowcurrently, @serhiy-storchaka, @tomdz, @miss-islington
PRs
  • bpo-13487: Use sys.modules.copy() in inspect for thread safety #18786
  • [3.8] bpo-13487: Use sys.modules.copy() in inspect.getmodule() for thread safety. (GH-18786) #18787
  • [3.7] bpo-13487: Use sys.modules.copy() in inspect.getmodule() for thread safety. (GH-18786) #18788
  • Files
  • getmodulefix.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/gpshead'
    closed_at = <Date 2020-03-05.01:07:10.959>
    created_at = <Date 2011-11-27.03:06:48.658>
    labels = ['3.7', '3.8', 'type-bug', 'library']
    title = 'inspect.getmodule fails when module imports change sys.modules'
    updated_at = <Date 2020-03-10.07:48:53.641>
    user = 'https://bugs.python.org/ErikTollerud'

    bugs.python.org fields:

    activity = <Date 2020-03-10.07:48:53.641>
    actor = 'ned.deily'
    assignee = 'gregory.p.smith'
    closed = True
    closed_date = <Date 2020-03-05.01:07:10.959>
    closer = 'gregory.p.smith'
    components = ['Library (Lib)']
    creation = <Date 2011-11-27.03:06:48.658>
    creator = 'Erik.Tollerud'
    dependencies = []
    files = ['23790']
    hgrepos = []
    issue_num = 13487
    keywords = ['patch']
    message_count = 20.0
    messages = ['148438', '148459', '148487', '148509', '148511', '148581', '148583', '299829', '301376', '302312', '302327', '302337', '363402', '363403', '363407', '363408', '363409', '363410', '363411', '363810']
    nosy_count = 12.0
    nosy_names = ['gregory.p.smith', 'amaury.forgeotdarc', 'ned.deily', 'eric.araujo', 'meador.inge', 'python-dev', 'eric.snow', 'Erik.Tollerud', 'serhiy.storchaka', 'psimons', 'tomdzk', 'miss-islington']
    pr_nums = ['18786', '18787', '18788']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'commit review'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue13487'
    versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

    @ErikTollerud
    Copy link
    Mannequin Author

    ErikTollerud mannequin commented Nov 27, 2011

    The inspect.getmodule function crashes if packages are installed that futz with sys.modules while they are being tested for module status or the like. I'm not actually sure which packages are doing this, but the symptom is the for loop over sys.modules raises an Exception because it is modified while the loop is running.

    This is *not* a problem in Python 2.x because sys.modules.items() returns a copy of the dictionary instead of an iterator, and 3.x changes that behavior. The comment above the for loop makes it clear that the expected behavior is a copy rather than an iterator, so the attached patch corrects the problem by simply wrapping the items() call in list().

    @ErikTollerud ErikTollerud mannequin added the stdlib Python modules in the Lib dir label Nov 27, 2011
    @amauryfa
    Copy link
    Member

    You are certainly right, but I wonder how this can happen.
    Are there modules which import something just by looking at them?
    Or is is some race condition due to another running thread?

    @merwok
    Copy link
    Member

    merwok commented Nov 28, 2011

    Maybe it can be caused by an installation happening during the loop. I agree with Erik’s reading of the comment and patch, and don’t think a test is needed.

    @merwok merwok added the type-bug An unexpected behavior, bug, or error label Nov 28, 2011
    @ErikTollerud
    Copy link
    Mannequin Author

    ErikTollerud mannequin commented Nov 28, 2011

    The package that triggers it for me is the py (http://pypi.python.org/pypi/py) package - when in gets imported, it does some trick with sys.modules that is in place to get around some pickling restriction, but that means sys.modules is altered during the import of the py package... and that triggers the exception.

    @amauryfa
    Copy link
    Member

    When a package is imported sys.modules changes... nothing special here.
    But it's true true that py.std, for example, is a "lazy" module with a special __getattr__ that will import submodules.

    Patch looks good to me as well.

    @merwok merwok self-assigned this Nov 29, 2011
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 29, 2011

    New changeset 2ef359d7a2e9 by Éric Araujo in branch '3.2':
    Fix inspect.getmodule to use a copy of sys.modules for iteration (bpo-13487).
    http://hg.python.org/cpython/rev/2ef359d7a2e9

    @merwok
    Copy link
    Member

    merwok commented Nov 29, 2011

    Committed, thanks.

    @merwok merwok closed this as completed Nov 29, 2011
    @psimons
    Copy link
    Mannequin

    psimons mannequin commented Aug 7, 2017

    list(sys.modules.items()) still raises RuntimeError: dictionary changed size during iteration when another thread imports a module.

    I would assume dict.copy() is thread-safe so a working fix could use sys.modules.copy().items()

    I hit this bug when printing the name of the caller function using
    inspect.stack(). The workaround in this case is calling inspect.stack(context=0).

    @tomdz
    Copy link
    Mannequin

    tomdz mannequin commented Sep 5, 2017

    This bug seems to be still there. I had an application fail with this same error in inspect.getouterframes with Python 3.6.2. As far as I could trace it, during iteration over sys.modules _tracemalloc and tracemalloc were added, not quite sure from where (maybe the warnings module ?)

    @merwok
    Copy link
    Member

    merwok commented Sep 16, 2017

    Could you give code to reproducer the problem, if possible without third-party dependencies?

    @merwok merwok added the 3.7 (EOL) end of life label Sep 16, 2017
    @merwok merwok reopened this Sep 16, 2017
    @serhiy-storchaka
    Copy link
    Member

    dict.copy() is not thread-safe still (but it can be made thread-safe).

    list(dict) is thread-safe. It copies a list of keys only.

    @psimons
    Copy link
    Mannequin

    psimons mannequin commented Sep 16, 2017

    I cannot reproduce. In fact I cannot even get list(d.items())
    to raise RuntimeError: dictionary changed size during iteration
    for any dict d.

    @gpshead
    Copy link
    Member

    gpshead commented Mar 4, 2020

    fyi - we just had a test run into this (in a flaky manner - definitely a race condition) at work:

    ...
        for f in inspect.stack(context=0)
      File "<embedded stdlib>/inspect.py", line 1499, in stack
        return getouterframes(sys._getframe(1), context)
      File "<embedded stdlib>/inspect.py", line 1476, in getouterframes
        frameinfo = (frame,) + getframeinfo(frame, context)
      File "<embedded stdlib>/inspect.py", line 1446, in getframeinfo
        filename = getsourcefile(frame) or getfile(frame)
      File "<embedded stdlib>/inspect.py", line 696, in getsourcefile
        if getattr(getmodule(object, filename), '__loader__', None) is not None:
      File "<embedded stdlib>/inspect.py", line 732, in getmodule
        for modname, module in list(sys.modules.items()):
    RuntimeError: dictionary changed size during iteration
    

    We haven't diagnosed what was leading to it though. Trust in the ability to use inspect.stack() -> ... -> inspect.getmodule() in multithreaded code is on the way out as a workaround.

    (this was on 3.6.7)

    A workaround we could checkin without consequences should be to change

    list(sys.modules.items()) into list(sys.modules.copy().items()).

    I was a bit surprised to see this happen at all, list(dict.items()) seems like it should've been done entirely in C with the GIL held the entire time. but maybe I'm just missing where the GIL would be released in the calls to exhause the iterator made by https://github.com/python/cpython/blob/master/Objects/listobject.c ?

    @gpshead
    Copy link
    Member

    gpshead commented Mar 4, 2020

    Serhiy: Why is dict.copy() not thread safe?

    if what you say of list(dict) being safe, iterating over that and looking up the keys would work. But all of this is entirely non-obvious to the reader of the code. all of these _look_ like they should be safe.

    We should make dict.copy() safe and document the guarantee as such as that one could at least be explained when used for that purpose.

    @gpshead
    Copy link
    Member

    gpshead commented Mar 5, 2020

    New changeset 85cf1d5 by Gregory P. Smith in branch 'master':
    bpo-13487: Use sys.modules.copy() in inspect.getmodule() for thread safety. (GH-18786)
    85cf1d5

    @gpshead
    Copy link
    Member

    gpshead commented Mar 5, 2020

    Testing by changing list(sys.modules.items()) to sys.modules.copy().items() internally with a large integration test that was reliably flaky on this line before shows that the .copy().items() worked. The test is reliable again.

    So I've gone ahead and pushed those changes in. PyDict_Copy()'s implementation at first ~5 minute glance did not appear to have calls to code I'd expect to re-enter Python releasing the GIL. But I didn't try to do a deep dive. It works for us and is logically equivalent.

    @miss-islington
    Copy link
    Contributor

    New changeset a123812 by Miss Islington (bot) in branch '3.7':
    bpo-13487: Use sys.modules.copy() in inspect.getmodule() for thread safety. (GH-18786)
    a123812

    @miss-islington
    Copy link
    Contributor

    New changeset 6b452ff by Miss Islington (bot) in branch '3.8':
    bpo-13487: Use sys.modules.copy() in inspect.getmodule() for thread safety. (GH-18786)
    6b452ff

    @gpshead
    Copy link
    Member

    gpshead commented Mar 5, 2020

    If anyone else has a way to reproduce this issue, please run your tests with a 3.7/3.8/3.9 interpreter with the fix I committed applied and report back if you still see this failure on that line.

    I believe this to be fixed based on my own testing so I am closing it out.

    @gpshead gpshead added the 3.8 only security fixes label Mar 5, 2020
    @gpshead gpshead closed this as completed Mar 5, 2020
    @gpshead gpshead assigned gpshead and unassigned merwok Mar 5, 2020
    @ned-deily
    Copy link
    Member

    New changeset 7058d2d by Ned Deily (Miss Islington (bot)) in branch '3.7':
    bpo-13487: Use sys.modules.copy() in inspect.getmodule() for thread safety. (GH-18786)
    7058d2d

    @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.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants