GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-28411: Support other mappings in PyInterpreterState.modules. by ericsnowcurrently · Pull Request #3593 · python/cpython

Conversation

ericsnowcurrently commented Sep 14, 2017 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ericsnowcurrently added 22 commits

September 14, 2017 18:20


          Add _PyImport_GetModule*.

8f3ecfc


          Allow sys.modules to be any mapping.

9dc9ded


          Add _PyImport_SetModule*.

f2a54b1


          Add PyImport_GetModule().

502e138


          Decref the module when done.

ec8c504


          Fix ref counts.

a1546f6


          Look up "new" modules in the given modules dict.

5a5fad1


          Fix error checking.

db0a247


          Use PyImport_GetModuleDict() in PyImport_GetModule().

7082b47


          Add a missing incref.

5082c47


          Add a Misc/NEWS entry.

bb03872


          Make the docs for PyImport_GetModule() more clear.

2e07296


          Fix style (bracket placement).


          Drop _PyImport_GetModuleString().

5b154b3


          Use PyDict_CheckExact() for sys.modules in fast case.

33174f3


          Do not use PyMapping_HasKey() with an error set.

e50e258


          Revert a code order change.

23cf493


          Switch the code order back.

eed5e24


          Only use PyDict_* for dicts.

bcc72a5


          Add a reference while cleaning up.

981dbd2


          Fix _pickle.

0fe1aff


          Fix the NEWS entry.

d4d9219

ericsnowcurrently requested a review from a team

September 14, 2017 23:32

bedevere-bot added the awaiting merge label

Sep 14, 2017

the-knights-who-say-ni added the CLA signed label

Sep 14, 2017

ericsnowcurrently force-pushed the sys-modules-any-mapping branch from 5a0b590 to d4d9219 Compare

September 14, 2017 23:33

ncoghlan reviewed

Sep 15, 2017

View reviewed changes

Comment thread

Python/import.c Outdated

		if (PyErr_ExceptionMatches(PyExc_KeyError)) {
		PyErr_Clear();
		}
		return mod;

ncoghlan Sep 15, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is missing the Py_XDECREF to return a borrowed reference instead of a new one.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ericsnowcurrently Sep 15, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Gah! The docs for PyDict_GetItemWithError() before 3.7 don't mention it returning a borrowed reference. Thanks for catching that. I was looking for that refleak!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ericsnowcurrently Sep 15, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Hmm. Looks like that wasn't the refleak I was looking for. :/ Back I go.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ncoghlan Sep 15, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I was just pattern matching with the code above based on the assumption that PyDict_GetItem and PyDict_GetItemWithError exposed the same refcount semantics :)

However, it now occurs to me that these access patterns are thoroughly dubious if you genuinely want to allow dict subclasses and arbitrary mappings, as those may return dynamic references (e.g. from __missing__ methods) , in which case the Py_XDECREF() line may invalidate the reference entirely.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ericsnowcurrently Sep 15, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Good call. I'll fix that.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ncoghlan reviewed

Sep 15, 2017

View reviewed changes

Comment thread

Python/import.c Outdated

		if (PyErr_Occurred()) {
		PyErr_Clear();
		}
		Py_XDECREF(mod);

ncoghlan Sep 15, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This could do with a second comment saying "Return a borrowed reference instead of a new one"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ericsnowcurrently Sep 15, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Will do.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Return a borrowed reference from _PyImport_GetModuleWithError().

9c39295

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ncoghlan reviewed

Sep 15, 2017

View reviewed changes

ncoghlan left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I suspect your reference leaks are coming from the iterators.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_pickle.c Outdated


		i = 0;
		while (PyDict_Next(modules_dict, &i, &module_name, &module)) {
		PyObject *iterator = PyObject_GetIter(modules);

ncoghlan Sep 15, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Where does this iterator get cleaned up?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ericsnowcurrently Sep 15, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thank you! I would have found it eventually but you just saved me a bunch of time!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/import.c

		}
		}
		else {
		PyObject *iterator = PyObject_GetIter(modules);

ncoghlan Sep 15, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This iterator doesn't appear to get cleaned up either.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ericsnowcurrently added 7 commits

September 15, 2017 19:27


          Fix some ref leaks.

446acd5


          Add a fast path for pickle.

540b99a


          Ignore errors from PyObject_GetItem().

5f0735b


          Factor out _checkmodule.

63beead


          Drop an extra INCREF.

00f3f24


          Drop _PyImport_GetModule() and _PyImport_GetModuleWithError().

5a7bba2


          tabs to spaces

3a73b52

ericsnowcurrently merged commit 3f9eee6 into python:master

Sep 15, 2017

ericsnowcurrently deleted the sys-modules-any-mapping branch

September 15, 2017 22:35

Mariatta removed the awaiting merge label

Oct 8, 2017

jeremycline mentioned this pull request

Jun 29, 2018

bpo-28411: Fix redundant declaration of _PyImport_AddModuleObject #7992

Merged

pganssle mentioned this pull request

Feb 5, 2019

bpo-34572: change _pickle unpickling to use import rather than retrieving from sys.modules #9047

Merged

ericsnowcurrently mentioned this pull request

Apr 10, 2022

[subinterpreters] Eliminate PyInterpreterState.modules. #72597

Open

colesbury mentioned this pull request

Nov 21, 2025

gh-137422: Fix race condition in PyImport_AddModuleRef #141822

Merged

Reviewers

ncoghlan

ncoghlan left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

ericsnowcurrently commented Sep 14, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently commented Sep 15, 2017

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ericsnowcurrently commented Sep 14, 2017 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.