◐ Shell
reader mode source ↗
Skip to content

bpo-28411: Support other mappings in PyInterpreterState.modules.#3593

Merged
ericsnowcurrently merged 30 commits into
python:masterfrom
ericsnowcurrently:sys-modules-any-mapping
Sep 15, 2017
Merged

bpo-28411: Support other mappings in PyInterpreterState.modules.#3593
ericsnowcurrently merged 30 commits into
python:masterfrom
ericsnowcurrently:sys-modules-any-mapping

Conversation

@ericsnowcurrently

@ericsnowcurrently ericsnowcurrently commented Sep 14, 2017 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

The concrete PyDict_* API is used to interact with PyInterpreterState.modules in a number of places. This isn't compatible with all dict subclasses, nor with other Mapping implementations. This patch switches the concrete API usage to the corresponding abstract API calls.

We also add a PyImport_GetModule() function (and some other helpers) to reduce a bunch of code duplication.

Note that this code was already reviewed and merged as part of #1638. I reverted that and am now splitting it up into more focused parts.

https://bugs.python.org/issue28411

@ericsnowcurrently
Add _PyImport_GetModule*.
8f3ecfc
@ericsnowcurrently
Allow sys.modules to be any mapping.
9dc9ded
@ericsnowcurrently
Add _PyImport_SetModule*.
f2a54b1
@ericsnowcurrently
Add PyImport_GetModule().
502e138
@ericsnowcurrently
Decref the module when done.
ec8c504
@ericsnowcurrently
Fix ref counts.
a1546f6
@ericsnowcurrently
Look up "new" modules in the given modules dict.
5a5fad1
@ericsnowcurrently
Fix error checking.
db0a247
@ericsnowcurrently
Use PyImport_GetModuleDict() in PyImport_GetModule().
7082b47
@ericsnowcurrently
Add a missing incref.
5082c47
@ericsnowcurrently
Add a Misc/NEWS entry.
bb03872
@ericsnowcurrently
Make the docs for PyImport_GetModule() more clear.
2e07296
@ericsnowcurrently
Fix style (bracket placement).
4631163
@ericsnowcurrently
Drop _PyImport_GetModuleString().
5b154b3
@ericsnowcurrently
Use PyDict_CheckExact() for sys.modules in fast case.
33174f3
@ericsnowcurrently
Do not use PyMapping_HasKey() with an error set.
e50e258
@ericsnowcurrently
Revert a code order change.
23cf493
@ericsnowcurrently
Switch the code order back.
eed5e24
@ericsnowcurrently
Only use PyDict_* for dicts.
bcc72a5
@ericsnowcurrently
Add a reference while cleaning up.
981dbd2
@ericsnowcurrently
Fix _pickle.
0fe1aff
@ericsnowcurrently
Fix the NEWS entry.
d4d9219
@ericsnowcurrently ericsnowcurrently requested a review from a team September 14, 2017 23:32
@ericsnowcurrently

ericsnowcurrently commented Sep 15, 2017

Copy link
Copy Markdown
Member Author

FYI, I'm hunting down a refleak (probably in import.c):

$ ./python -m test -R 3:3 test_capi -m test.test_capi.SubinterpreterTest.test_subinterps
Run tests sequentially
0:00:00 load avg: 0.07 [1/1] test_capi
beginning 6 repetitions
123456
......
test_capi leaked [2, 2, 2] references, sum=6
test_capi leaked [1, 2, 1] memory blocks, sum=4
test_capi failed

1 test failed:
    test_capi

Total duration: 220 ms
Tests result: FAILURE

test_pickle hits it too. Once I track it down and fix it I'm planning on merging.

ncoghlan
ncoghlan reviewed Sep 15, 2017
View reviewed changes

@ncoghlan ncoghlan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

I suspect your reference leaks are coming from the iterators.

@ericsnowcurrently ericsnowcurrently merged commit 3f9eee6 into python:master Sep 15, 2017
@ericsnowcurrently ericsnowcurrently deleted the sys-modules-any-mapping branch September 15, 2017 22:35
@jeremycline jeremycline mentioned this pull request Jun 29, 2018
Merged
@pganssle pganssle mentioned this pull request Feb 5, 2019
Merged
@ericsnowcurrently ericsnowcurrently mentioned this pull request Apr 10, 2022
Open
@colesbury colesbury mentioned this pull request Nov 21, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ncoghlan ncoghlan ncoghlan left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ericsnowcurrently @ncoghlan @Mariatta @the-knights-who-say-ni @bedevere-bot