Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-91351: Fix some bugs in importlib handling of re-entrant imports #94504

Merged
merged 35 commits into from
Jan 21, 2023

Conversation

exarkun
Copy link
Contributor

@exarkun exarkun commented Jul 1, 2022

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()

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.
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.
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.
@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
@rpurdie
Copy link

rpurdie commented Aug 31, 2022

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?

@brettcannon
Copy link
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.

Lib/importlib/_bootstrap.py Outdated Show resolved Hide resolved
Lib/importlib/_bootstrap.py Outdated Show resolved Hide resolved
Lib/importlib/_bootstrap.py Outdated Show resolved Hide resolved
Lib/importlib/_bootstrap.py Outdated Show resolved Hide resolved
Lib/importlib/_bootstrap.py Outdated Show resolved Hide resolved
Lib/importlib/_bootstrap.py Outdated Show resolved Hide resolved
Lib/importlib/_bootstrap.py Outdated Show resolved Hide resolved
Lib/importlib/_bootstrap.py Outdated Show resolved Hide resolved
Lib/importlib/_bootstrap.py Outdated Show resolved Hide resolved
Lib/importlib/_bootstrap.py Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

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.

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

Lib/importlib/_bootstrap.py Outdated Show resolved Hide resolved
Lib/importlib/_bootstrap.py Outdated Show resolved Hide resolved
Lib/importlib/_bootstrap.py Outdated Show resolved Hide resolved
Copy link
Member

@brettcannon brettcannon left a 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!

@bedevere-bot
Copy link

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.

Co-authored-by: Brett Cannon <brett@python.org>
@exarkun
Copy link
Contributor Author

exarkun commented Jan 14, 2023

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@brettcannon
Copy link
Member

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

facebook-github-bot pushed a commit to facebookincubator/cinder that referenced this pull request Feb 12, 2024
…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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

importlib lock race issue in deadlock handling code
4 participants