GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-28411: Remove PyInterpreterState.modules. by ericsnowcurrently · Pull Request #3606 · python/cpython

Conversation

@ericsnowcurrently

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

Uh oh!

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

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
ericsnowcurrently added 6 commits September 15, 2017 22:36
@ericsnowcurrently
Look up sys.modules in PyImport_GetModuleDict().
3920902
@ericsnowcurrently
Check sys.modules in _PyImport_EnsureInitialized().
9c8f4dd
@ericsnowcurrently
Drop PyInterpreterState.modules.
4e97cbf
@ericsnowcurrently
Add a Misc/NEWS entry.
19694f3
@ericsnowcurrently
Add a whatsnew entry for porting.
548428c
@ericsnowcurrently
Use PyImport_GetModuleDict() in pickle.
b31768e
@ericsnowcurrently
ericsnowcurrently requested a review from a team September 15, 2017 22:51
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Sep 15, 2017
@bedevere-bot bedevere-bot added the awaiting merge label Sep 15, 2017
@ericsnowcurrently
ericsnowcurrently requested a review from vstinner September 15, 2017 22:52
@vstinner vstinner changed the title bpo-28411: Remove PyInterpreterState.modules. [WIP] bpo-28411: Remove PyInterpreterState.modules. Sep 15, 2017
@vstinner

vstinner commented Sep 15, 2017

Copy link
Copy Markdown
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."
All reactions

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently commented Sep 15, 2017

Copy link
Copy Markdown
Member Author

Thanks.
All reactions

Sorry, something went wrong.

Uh oh!

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

brettcannon commented Mar 23, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently ericsnowcurrently mentioned this pull request May 23, 2018
bpo-33330: PyImport_Cleanup check for exc leaks #7068
Closed
@brettcannon brettcannon added the DO-NOT-MERGE label Mar 1, 2019
@ericsnowcurrently ericsnowcurrently changed the title [WIP] bpo-28411: Remove PyInterpreterState.modules. bpo-28411: Remove PyInterpreterState.modules. Mar 1, 2019
@ericsnowcurrently

ericsnowcurrently commented Mar 1, 2019

Copy link
Copy Markdown
Member Author

FYI, I'm going to revisit this PR in the near future.
auvipy reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brettcannon
brettcannon removed the request for review from a team April 17, 2019 22:44
@brettcannon

brettcannon commented Apr 17, 2019

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

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

matrixise commented Sep 17, 2019

Copy link
Copy Markdown
Member

Hi @ericsnowcurrently

Do you continue to work on this PR?

Thanks
All reactions

Sorry, something went wrong.

Uh oh!

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

alimcmaster1 commented Apr 11, 2020

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently commented Apr 11, 2020 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

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

alimcmaster1 commented Apr 11, 2020

Copy link
Copy Markdown

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions

github-actions Bot commented Feb 22, 2022

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Feb 22, 2022
@ericsnowcurrently ericsnowcurrently mentioned this pull request Apr 10, 2022
[subinterpreters] Eliminate PyInterpreterState.modules. #72597
Open
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Jul 29, 2022
@github-actions

github-actions Bot commented Aug 31, 2022

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Aug 31, 2022
@ericsnowcurrently

ericsnowcurrently commented Aug 31, 2022

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently ericsnowcurrently closed this Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner Awaiting requested review from vstinner

Uh oh!

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

Assignees

No one assigned

Labels

awaiting merge DO-NOT-MERGE stale Stale PR or inactive for long period of time.

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.

8 participants

@ericsnowcurrently @vstinner @brettcannon @matrixise @alimcmaster1 @the-knights-who-say-ni @ezio-melotti @bedevere-bot
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.