Skip to content

Conversation

@ericsnowcurrently
Copy link
Member

@ericsnowcurrentlyericsnowcurrently commented Sep 15, 2017

The status quo for sys.modules is broken. While importlib (and a few other places) use sys.modules directly, the C-API (including PyImport_Import(), etc.) use PyInterpreterState.modules directly. Since assigning to sys.modules does not update PyInterpreterState.modules, the two can get out of sync. The workaround, which we use in the test suite, is complicated. This patch resolves the issue by getting rid of PyInterpreterState.modules and making sys.modules authoritative in the C-API.

Note that this code was landed earlier (PR #1638), but reverted due to problems when a bogus value (e.g. []) is assigned to sys.modules (see bpo-31404), which is very much a corner case. I've broken that original patch up and already landed the majority. The remaining piece here is to actually drop PyInterpreterState.modules. This PR should not be merged until the problem described in bpo-31404 is resolved.

https://bugs.python.org/issue28411

@ericsnowcurrentlyericsnowcurrently requested a review from a teamSeptember 15, 2017 22:51
@vstinnervstinner changed the title bpo-28411: Remove PyInterpreterState.modules.[WIP] bpo-28411: Remove PyInterpreterState.modules.Sep 15, 2017
@vstinner
Copy link
Member

I added [WIP] to your PR title for this reason: "This PR should not be merged until the problem described in bpo-31404 is resolved."

@ericsnowcurrently
Copy link
MemberAuthor

Thanks.

@brettcannon
Copy link
Member

brettcannon commented Mar 23, 2018

@ericsnowcurrently It looks like bpo-31404 was resolved, so can the WIP label be removed?

@ericsnowcurrentlyericsnowcurrently changed the title [WIP] bpo-28411: Remove PyInterpreterState.modules.bpo-28411: Remove PyInterpreterState.modules.Mar 1, 2019
@ericsnowcurrently
Copy link
MemberAuthor

FYI, I'm going to revisit this PR in the near future.

@brettcannonbrettcannon removed the request for review from a teamApril 17, 2019 22:44
@brettcannon
Copy link
Member

Removing the import team from reviewing as this is still a WIP. We can be added back when it's ready to go.

@matrixise
Copy link
Member

Hi @ericsnowcurrently

Do you continue to work on this PR?

Thanks

@alimcmaster1
Copy link

@matrixise -> think we should be good to close. No response from @ericsnowcurrently

@ericsnowcurrently
Copy link
MemberAuthor

ericsnowcurrently commented Apr 11, 2020

Does the problem described in bpo-31404 still exist with this branch applied?

@alimcmaster1
Copy link

Unsure - I couldn’t repro on Linux and don’t have a windows machine handy to check.

We can still keep the issue open - but might make sense to close this PR if you are no longer are interested in working on this. Unless you/others disagree?

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actionsgithub-actionsbot added the stale Stale PR or inactive for long period of time. label Feb 22, 2022
@github-actionsgithub-actionsbot removed the stale Stale PR or inactive for long period of time. label Jul 29, 2022
@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actionsgithub-actionsbot added the stale Stale PR or inactive for long period of time. label Aug 31, 2022
@ericsnowcurrently
Copy link
MemberAuthor

I'm fine with closing this. I'll probably circle back to it at some point.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting mergeDO-NOT-MERGEstaleStale PR or inactive for long period of time.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@ericsnowcurrently@vstinner@brettcannon@matrixise@alimcmaster1@the-knights-who-say-ni@ezio-melotti@bedevere-bot