classification
Title: inspect.getmodule fails when module imports change sys.modules
Type: behavior Stage: commit review
Components: Library (Lib) Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: Erik.Tollerud, amaury.forgeotdarc, eric.araujo, eric.snow, gregory.p.smith, meador.inge, miss-islington, ned.deily, psimons, python-dev, serhiy.storchaka, tomdzk
Priority: normal Keywords: patch

Created on 2011-11-27 03:06 by Erik.Tollerud, last changed 2020-03-10 07:48 by ned.deily. This issue is now closed.

Files
File name Uploaded Description Edit
getmodulefix.patch Erik.Tollerud, 2011-11-27 03:06 review
Pull Requests
URL Status Linked Edit
PR 18786 merged gregory.p.smith, 2020-03-05 00:13
PR 18787 merged miss-islington, 2020-03-05 00:45
PR 18788 merged miss-islington, 2020-03-05 00:45
Messages (20)
msg148438 - (view) Author: Erik Tollerud (Erik.Tollerud) Date: 2011-11-27 03:06
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().
msg148459 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2011-11-27 21:46
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?
msg148487 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-11-28 15:08
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.
msg148509 - (view) Author: Erik Tollerud (Erik.Tollerud) Date: 2011-11-28 18:37
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.
msg148511 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2011-11-28 18:52
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.
msg148581 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2011-11-29 16:15
New changeset 2ef359d7a2e9 by Éric Araujo in branch '3.2':
Fix inspect.getmodule to use a copy of sys.modules for iteration (#13487).
http://hg.python.org/cpython/rev/2ef359d7a2e9
msg148583 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-11-29 16:19
Committed, thanks.
msg299829 - (view) Author: Patrik Simons (psimons) Date: 2017-08-07 07:57
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).
msg301376 - (view) Author: Thomas Dudziak (tomdzk) Date: 2017-09-05 21:12
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 ?)
msg302312 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2017-09-16 02:32
Could you give code to reproducer the problem, if possible without third-party dependencies?
msg302327 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-16 06:24
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.
msg302337 - (view) Author: Patrik Simons (psimons) Date: 2017-09-16 12:09
I cannot reproduce. In fact I cannot even get list(d.items())
to raise RuntimeError: dictionary changed size during iteration
for any dict d.
msg363402 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2020-03-04 22:57
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 ?
msg363403 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2020-03-04 23:00
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.
msg363407 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2020-03-05 00:45
New changeset 85cf1d514b84dc9a4bcb40e20a12e1d82ff19f20 by Gregory P. Smith in branch 'master':
bpo-13487: Use sys.modules.copy() in inspect.getmodule() for thread safety. (GH-18786)
https://github.com/python/cpython/commit/85cf1d514b84dc9a4bcb40e20a12e1d82ff19f20
msg363408 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2020-03-05 00:50
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.
msg363409 - (view) Author: miss-islington (miss-islington) Date: 2020-03-05 01:03
New changeset a12381233a243ba7d5151ebf060feb57dd540bef by Miss Islington (bot) in branch '3.7':
bpo-13487: Use sys.modules.copy() in inspect.getmodule() for thread safety. (GH-18786)
https://github.com/python/cpython/commit/a12381233a243ba7d5151ebf060feb57dd540bef
msg363410 - (view) Author: miss-islington (miss-islington) Date: 2020-03-05 01:04
New changeset 6b452ff97f70eca79ab956987cc04b6586feca00 by Miss Islington (bot) in branch '3.8':
bpo-13487: Use sys.modules.copy() in inspect.getmodule() for thread safety. (GH-18786)
https://github.com/python/cpython/commit/6b452ff97f70eca79ab956987cc04b6586feca00
msg363411 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2020-03-05 01:07
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.
msg363810 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2020-03-10 07:48
New changeset 7058d2d96c5ca4dfc6c754c5cd737c6eb2a8fd67 by Ned Deily (Miss Islington (bot)) in branch '3.7':
bpo-13487: Use sys.modules.copy() in inspect.getmodule() for thread safety. (GH-18786)
https://github.com/python/cpython/commit/7058d2d96c5ca4dfc6c754c5cd737c6eb2a8fd67
History
Date User Action Args
2020-03-10 07:48:53ned.deilysetnosy: + ned.deily
messages: + msg363810
2020-03-05 01:07:10gregory.p.smithsetstatus: open -> closed
versions: + Python 3.8
messages: + msg363411

assignee: eric.araujo -> gregory.p.smith
resolution: fixed
stage: patch review -> commit review
2020-03-05 01:04:42miss-islingtonsetmessages: + msg363410
2020-03-05 01:03:35miss-islingtonsetmessages: + msg363409
2020-03-05 00:50:55gregory.p.smithsetmessages: + msg363408
2020-03-05 00:45:39miss-islingtonsetpull_requests: + pull_request18145
2020-03-05 00:45:36gregory.p.smithsetmessages: + msg363407
2020-03-05 00:45:33miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request18144
2020-03-05 00:13:42gregory.p.smithsetstage: resolved -> patch review
pull_requests: + pull_request18143
2020-03-04 23:00:59gregory.p.smithsetmessages: + msg363403
2020-03-04 22:57:53gregory.p.smithsetnosy: + gregory.p.smith
messages: + msg363402
2017-09-16 12:09:04psimonssetmessages: + msg302337
2017-09-16 06:24:41serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg302327
2017-09-16 02:32:57eric.araujosetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg302312

versions: + Python 3.6, Python 3.7, - Python 3.2, Python 3.3
2017-09-05 21:12:00tomdzksetnosy: + tomdzk
messages: + msg301376
2017-08-07 07:57:52psimonssetnosy: + psimons
messages: + msg299829
2011-11-29 16:19:19eric.araujosetstatus: open -> closed
resolution: fixed
messages: + msg148583

stage: patch review -> resolved
2011-11-29 16:15:06python-devsetnosy: + python-dev
messages: + msg148581
2011-11-29 15:37:31eric.araujosetassignee: eric.araujo
2011-11-28 18:52:47amaury.forgeotdarcsetmessages: + msg148511
2011-11-28 18:37:46Erik.Tollerudsetmessages: + msg148509
2011-11-28 15:08:04eric.araujosetversions: - Python 3.1
nosy: + eric.araujo

messages: + msg148487

type: behavior
stage: patch review
2011-11-27 21:46:56amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg148459
2011-11-27 17:18:55meador.ingesetnosy: + meador.inge
2011-11-27 03:13:56eric.snowsetnosy: + eric.snow
2011-11-27 03:06:48Erik.Tollerudcreate