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 — gh-91351: Fix some bugs in importlib handling of re-entrant imports by exarkun · Pull Request #94504 · python/cpython

Conversation

@exarkun

@exarkun exarkun commented Jul 1, 2022

Copy link
Copy Markdown
Contributor

Fixes #91351
Update of #94342

See #91351 for details about the problem.

Re-entrancy is always tricky and given the requirements of _bootstrap.py (to operate with re-entrancy and multi-threading and to do so without exposing any of the details to application code doing an import) I think this goes double.

This PR does a few things to achieve better safety in the face of re-entrancy:

  • Switch some data structures to those that support atomic operation so that they are consistent in case of asynchronous re-entrancy (eg from the garbage collector or a signal handler).
  • Use RLock and add more RLock-like behavior to prevent deadlocks in the re-entrant case.
  • Update the deadlock detection algorithm to support the fact that one thread might be "blocked on" acquiring the module lock for more than one module at a time.

I'm not quite sure I believe this new version of the code is 100% correct with respect to re-entrancy but it does fixes mishandling of two specific cases:

  • A re-entrant import is performed between the time _blocking_on is populated and cleaned up inside _ModuleLock.acquire. Previous this failed with a KeyError (as described in the linked issue).
  • A re-entrant import is performed while the module lock is held inside _ModuleLock.acquire. Previously this failed by deadlocking.

This PR also does not include any new unit tests. I have a small stand-alone program which can reproduce both of these but only with the assistance of some additional instrumentation inside _bootstrap.py to make sure the re-entrancy happens at the interesting times. If adding this kind of instrumentation is acceptable then it may be possible to turn this program into some unit tests.

Compared to the previous PR, this one is a bit simpler because it uses RLock in a place where main currently uses a regular Lock.

For reference, here is a stand-alone reproducer. This one very reliably reproduces the KeyError problem (by exercising the codepath forever until it hits it). I haven't managed to create a deterministic reproducer for the deadlock case - except with the assistance of instrumentation inside _bootstrap.py itself.

import sys, socket, gc

class Cycle:
    pass

def a_cycle():
    c = Cycle()
    c.cycle = c
    c.s = socket.socket()

def main():
    while True:
        # import a module that socket.__del__ is going to import to exercise
        # re-entrant _ModuleLock.lock handling
        a_cycle()
        import linecache

        del sys.modules["linecache"]

main()
  • Issue: importlib lock race issue in deadlock handling code #91351

Sorry, something went wrong.

Uh oh!

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

All reactions
exarkun added 3 commits June 30, 2022 18:58
@exarkun
Add some implementation documentation
2a4d4f3
This might make it a little bit easier for a new reader/maintainer to
understand what this code is doing.

Also, link to a couple application-level bug reports about probable
misbehaviors related to the deadlock-detection code.
@exarkun
Move _blocking_on management into a context manager
c9e33c7
This more clearly separates the logic for that management from the application
code that runs in this context.

It will also make subsequent changes to improve that logic more clear.
@exarkun
Support re-entrant imports in _BlockingOnManager
c5912bf
Also update the deadlock detection to work with the new _blocking_on

Switch to a recursive implementation so it can easily follow the branching
path through the "blocking on" graph that is now possible thanks to
re-entrancy.
@bedevere-bot bedevere-bot added the awaiting review label Jul 1, 2022
@exarkun exarkun mentioned this pull request Jul 1, 2022
importlib lock race issue in deadlock handling code #91351
Closed
@brettcannon brettcannon changed the title gn-91351: Fix some bugs in importlib handling of re-entrant imports gh-91351: Fix some bugs in importlib handling of re-entrant imports Jul 4, 2022
@brettcannon
brettcannon requested review from brettcannon, ericsnowcurrently and pitrou July 4, 2022 22:52
@rpurdie

rpurdie commented Aug 31, 2022

Copy link
Copy Markdown

For what it is worth, Yocto Project keep running into issues caused by re-entrant imports as mentioned in issue #91351. I believe that issue references a couple of older issues where people have run into this too. Is it possible to review this and get it resolved?

All reactions

Sorry, something went wrong.

Uh oh!

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

@hsmett hsmett mentioned this pull request Sep 2, 2022
Test Suite faillure: importlib race condition HybirdCorp/creme_crm#430
Closed
@brettcannon

brettcannon commented Sep 23, 2022

Copy link
Copy Markdown
Member

@rpurdie I do plan to review it, but it is not at the top of my review queue yet. If you want to provide a review yourself then that would be appreciated.

All reactions

Sorry, something went wrong.

Uh oh!

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

brettcannon
brettcannon requested changes Nov 25, 2022
View reviewed changes
Comment thread Lib/importlib/_bootstrap.py Resolved

Uh oh!

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

Comment thread Lib/importlib/_bootstrap.py Resolved

Uh oh!

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

Comment thread Lib/importlib/_bootstrap.py Resolved

Uh oh!

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

Comment thread Lib/importlib/_bootstrap.py Resolved

Uh oh!

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

Comment thread Lib/importlib/_bootstrap.py Resolved

Uh oh!

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

Comment thread Lib/importlib/_bootstrap.py Resolved

Uh oh!

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

Comment thread Lib/importlib/_bootstrap.py Resolved

Uh oh!

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

Comment thread Lib/importlib/_bootstrap.py Resolved

Uh oh!

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

Comment thread Lib/importlib/_bootstrap.py Resolved

Uh oh!

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

Comment thread Lib/importlib/_bootstrap.py Resolved

Uh oh!

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

@bedevere-bot bedevere-bot added awaiting changes and removed awaiting review labels Nov 25, 2022
@bedevere-bot

bedevere-bot commented Nov 25, 2022

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

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

exarkun and others added 14 commits November 28, 2022 13:52
@exarkun @brettcannon
Apply formatting and comment changes from review
27ae2f0
Co-authored-by: Brett Cannon <[email protected]>
@exarkun
Merge remote-tracking branch 'origin/main' into 91351-importlib-reent…
1fcc78b
…rancy
@exarkun
Rename _BlockingOnManager.tid as suggested by review
5fd15d8
@exarkun
flip the first two arguments to _has_deadlock as suggested by review
7a24f2c
Also rename it as suggested by review
@exarkun
Mark up parameters following PEP 257 as suggested by review
08892b4
See https://peps.python.org/pep-0257/#multi-line-docstrings
@exarkun
rename the _blocking_on parameter as suggested by review
59b53c0
@exarkun
further document motivation for blocking_on parameter as suggested …
20007c5
…by review
@exarkun
Rename more _has_deadlocked parameters as suggested by review
bad1d3c
@exarkun
Treat None and [] the same for this case as suggested by review
2821fcf
@exarkun
update old comment to refer to new names as suggested by review
95c73cb
@exarkun
Make _ModuleLock.count a list of True as suggested by review
49ff9dd
@exarkun @brettcannon
Adjust the check for a module lock being released as suggest by review
cd174a8
Co-authored-by: Brett Cannon <[email protected]>
@exarkun
Finish the _BlockingOnManager.tid renaming
719b181
@exarkun
Fix renaming of _blocking_on parameter to _has_deadlocked
6e809cd
@bedevere-bot bedevere-bot added the awaiting change review label Jan 6, 2023
@bedevere-bot

bedevere-bot commented Jan 6, 2023

Copy link
Copy Markdown

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot
bedevere-bot requested a review from brettcannon January 6, 2023 23:39
brettcannon
brettcannon approved these changes Jan 14, 2023
View reviewed changes
Comment thread Lib/importlib/_bootstrap.py Resolved

Uh oh!

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

Comment thread Lib/importlib/_bootstrap.py Resolved

Uh oh!

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

Comment thread Lib/importlib/_bootstrap.py Resolved

Uh oh!

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

@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Jan 14, 2023
brettcannon
brettcannon requested changes Jan 14, 2023
View reviewed changes

@brettcannon brettcannon left a comment

Copy link
Copy Markdown
Member

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 would have approved and made the commits myself, but the branch is still blocked from me making changes. 😅 Can you just make the last couple and then I can merge this!

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting merge labels Jan 14, 2023
@bedevere-bot

bedevere-bot commented Jan 14, 2023

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

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

@exarkun @brettcannon
Apply suggestions from code review
b35f0a8
Co-authored-by: Brett Cannon <[email protected]>
@exarkun

exarkun commented Jan 14, 2023

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

rpurdie 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.

@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Jan 14, 2023
@bedevere-bot

bedevere-bot commented Jan 14, 2023

Copy link
Copy Markdown

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot
bedevere-bot requested a review from brettcannon January 14, 2023 13:09
brettcannon
brettcannon approved these changes Jan 20, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Jan 20, 2023
@brettcannon
brettcannon merged commit 3325f05 into python:main Jan 21, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Jan 21, 2023
@brettcannon

brettcannon commented Jan 21, 2023

Copy link
Copy Markdown
Member

Thanks so much for the PR and sticking with it until it got in!

rpurdie 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.

@corona10 corona10 mentioned this pull request Feb 15, 2023
Refleak in test_importlib on aarch64 RHEL8 #101766
Closed
@exarkun exarkun mentioned this pull request Feb 17, 2023
gh-101766: Fix refleak for _BlockingOnManager resources #101942
Merged
@kairu-ms kairu-ms mentioned this pull request Apr 27, 2023
Az 2.48.1 ImportError: cannot import name 'Request' from partially initialized module 'requests.models' Azure/azure-cli#26272
Closed
This was referenced May 21, 2023
test_pickle are leaked on Windows #104702
Closed
gh-102251: add missing cleanups for test_import #104796
Closed
@sunmy2019 sunmy2019 mentioned this pull request Jun 28, 2023
test_import.test_concurrency leaks ref on Windows #106176
Closed
@Eclips4 Eclips4 mentioned this pull request Jun 28, 2023
gh-106176: Fix reference leak in importlib/_bootstrap.py #106207
Closed
facebook-github-bot pushed a commit to facebookincubator/MetaPython that referenced this pull request Feb 12, 2024
@itamaro @facebook-github-bot
Backport gh-91351: Fix some bugs in importlib handling of re-entrant …
4dd8504
…imports

Summary:
upstream issue: python/cpython#91351
upstream PR: python/cpython#94504
upstream merge commit: python/cpython@3325f05

symptom:

```
File "<frozen importlib._bootstrap>", line 1004, in _find_and_load(name='oe.gpg_sign', import_=<built-in function __import__>)
File "<frozen importlib._bootstrap>", line 158, in _ModuleLockManager.__enter__()
File "<frozen importlib._bootstrap>", line 110, in _ModuleLock.acquire()
KeyError: 139622474778432

and

File "<frozen importlib._bootstrap>", line 1004, in _find_and_load(name='oe.path', import_=<built-in function __import__>)
File "<frozen importlib._bootstrap>", line 158, in _ModuleLockManager.__enter__()
File "<frozen importlib._bootstrap>", line 110, in _ModuleLock.acquire()
KeyError: 140438942700992
```

Reviewed By: carljm

Differential Revision: D53641441

fbshipit-source-id: e142eb17442da370861cd3a3398b0eef9930d041
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brettcannon brettcannon brettcannon approved these changes
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@pitrou pitrou Awaiting requested review from pitrou

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.

importlib lock race issue in deadlock handling code

Uh oh!

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

4 participants

@exarkun @rpurdie @brettcannon @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 01402520 — 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.