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-114763: Protect lazy loading modules from attribute access race by effigies · Pull Request #114781 · python/cpython

Conversation

@effigies

@effigies effigies commented Jan 31, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

As described in #114763, setting the __class__ attribute of a lazy-loading module to ModuleType enables other threads to attempt to access attributes before the loading is complete. This PR moves self.__class__ = types.ModuleType to be the final act.

This requires two additional pieces to work:

  1. A lock, followed by a __class__ check to prevent threads that arrive while the module is being loaded from attempting to double-load the module.
  2. An Event to indicate that the load is in progress to the loading thread, so it can permit dunder attribute access to the exec_module() call. This path also requires that the lock be reentrant.

The Event needs to be tied to the specific module, so I also tied the lock to the module, as opposed to a lock scoped to the importlib.util module. I used object.__getattribute__() for __spec__ and __dict__, which were previously accessed directly after self.__class__ was reset.

Otherwise, I tried to keep things as close as possible to the original. I will try to write a unit test fitting with the module style, but the minimal reproduction in the issue is resolved by these changes.

Closes #114763

  • Issue: Accessing attributes of a lazily-loaded module is not thread-safe #114763

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
stefanv reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@effigies
effigies requested review from brettcannon, ericsnowcurrently, ncoghlan and warsaw as code owners January 31, 2024 03:43
@bedevere-app bedevere-app Bot mentioned this pull request Jan 31, 2024
Accessing attributes of a lazily-loaded module is not thread-safe #114763
Closed
@bedevere-app bedevere-app Bot added the awaiting review label Jan 31, 2024
brettcannon
brettcannon reviewed Jan 31, 2024
View reviewed changes
Comment thread Lib/importlib/util.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/importlib/util.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/importlib/util.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_importlib/test_lazy.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@effigies effigies mentioned this pull request Feb 1, 2024
Broken thread safety scientific-python/lazy-loader#88
Closed
effigies added 5 commits February 5, 2024 09:28
@effigies
pythongh-114763: Protect lazy loading modules from attribute access race
d57f4eb
@effigies
TEST: Reproduce pythongh-114763
3db717c
@effigies
Add new blurb
5424d0c
@effigies
Improve comments around the branching
96c08c5
@effigies
TEST: Use TestCase.assert* methods
3642ddc
@effigies
effigies force-pushed the fix-issue-114763 branch from dded16c to 3642ddc Compare February 5, 2024 14:29
@brettcannon
Merge branch 'main' into fix-issue-114763
46238e5
@effigies

effigies commented Feb 12, 2024

Copy link
Copy Markdown
Contributor Author

@brettcannon Just checking in if you're waiting on me for anything. (No rush from my end.)
All reactions

Sorry, something went wrong.

Uh oh!

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

brettcannon commented Feb 13, 2024

Copy link
Copy Markdown
Member

@effigies Nope, I'm just swamped right now, so I haven't had time to do another review yet.
effigies 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 self-requested a review February 13, 2024 00:29
@effigies

effigies commented Feb 13, 2024

Copy link
Copy Markdown
Contributor Author

No worries! I appreciate you taking the time, whenever you get it.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
brettcannon
brettcannon requested changes Feb 16, 2024
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.

Some minor tweaks, but otherwise LGTM!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/importlib/util.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/importlib/util.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/importlib/util.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/importlib/util.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_importlib/test_lazy.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the awaiting review label Feb 16, 2024
@bedevere-app

bedevere-app Bot commented Feb 16, 2024

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.
@bedevere-app bedevere-app Bot added the awaiting changes label Feb 16, 2024
effigies and others added 3 commits February 16, 2024 19:32
@effigies @brettcannon
Apply suggestions from code review
6accf0c 
Co-authored-by: Brett Cannon <[email protected]>
@effigies
TEST: Require working threading for test_module_load_race
bb2292f
@effigies
Move threading import to module level of importlib.util
57fe084
@effigies

effigies commented Feb 17, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

@bedevere-bot I have made the requested changes; please review again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aisk
aisk reviewed Feb 17, 2024
View reviewed changes
Comment thread Lib/importlib/util.py Outdated
loader_state['__dict__'] = module.__dict__.copy()
loader_state['__class__'] = module.__class__
loader_state['lock'] = threading.RLock()
loader_state['is_loading'] = threading.Event()

@aisk aisk Feb 17, 2024

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.

Hi, can we use a bool flag instead of threading.Event? I see that access and modification to loader_state['is_loading'] is protected by the loader_state['lock'], so there is no thread safety issue.

Using an additional threading.Event would introduce unnecessary resource costs.

Sorry, something went wrong.

Uh oh!

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

@effigies effigies Feb 17, 2024

Copy link
Copy Markdown
Contributor 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.

I agree, good catch. I will switch to a bool and push later today.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aisk reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
effigies added 2 commits February 17, 2024 10:15
@effigies
Make is_loading a bool instead of Event
40383a0
@effigies
Merge branch 'main' into fix-issue-114763
6322dd6
@brettcannon

brettcannon commented Feb 23, 2024

Copy link
Copy Markdown
Member

!buildbot wasi
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Feb 23, 2024

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @brettcannon for commit 023d65d 🤖

The command will test the builders whose names match following regular expression: wasi

The builders matched are:

  • wasm32-wasi Non-Debug PR
  • wasm32 WASI 8Core PR
  • wasm32-wasi PR
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brettcannon brettcannon added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes type-bug An unexpected behavior, bug, or error topic-importlib labels Feb 23, 2024
brettcannon
brettcannon approved these changes Feb 23, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting changes labels Feb 23, 2024
@brettcannon
brettcannon merged commit 200271c into python:main Feb 24, 2024
@miss-islington-app

miss-islington-app Bot commented Feb 24, 2024

Copy link
Copy Markdown

Thanks @effigies for the PR, and @brettcannon for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 24, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 24, 2024
@effigies @miss-islington
pythongh-114763: Protect lazy loading modules from attribute access r…
21221d5 
…aces (pythonGH-114781)

Setting the __class__ attribute of a lazy-loading module to ModuleType enables other threads to attempt to access attributes before the loading is complete. Now that is protected by a lock.
(cherry picked from commit 200271c)

Co-authored-by: Chris Markiewicz <[email protected]>
@bedevere-app

bedevere-app Bot commented Feb 24, 2024

Copy link
Copy Markdown

GH-115870 is a backport of this pull request to the 3.12 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Feb 24, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 24, 2024
@effigies @miss-islington
pythongh-114763: Protect lazy loading modules from attribute access r…
97d883b 
…aces (pythonGH-114781)

Setting the __class__ attribute of a lazy-loading module to ModuleType enables other threads to attempt to access attributes before the loading is complete. Now that is protected by a lock.
(cherry picked from commit 200271c)

Co-authored-by: Chris Markiewicz <[email protected]>
@bedevere-app

bedevere-app Bot commented Feb 24, 2024

Copy link
Copy Markdown

GH-115871 is a backport of this pull request to the 3.11 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.11 only security fixes label Feb 24, 2024
@effigies
effigies deleted the fix-issue-114763 branch February 24, 2024 00:03
brettcannon pushed a commit that referenced this pull request Feb 26, 2024
@miss-islington @effigies
[3.12] gh-114763: Protect lazy loading modules from attribute access …
7b91b90 
…races (GH-114781) (GH-115870)

gh-114763: Protect lazy loading modules from attribute access races (GH-114781)

Setting the __class__ attribute of a lazy-loading module to ModuleType enables other threads to attempt to access attributes before the loading is complete. Now that is protected by a lock.
(cherry picked from commit 200271c)

Co-authored-by: Chris Markiewicz <[email protected]>
brettcannon pushed a commit that referenced this pull request Feb 26, 2024
@miss-islington @effigies
[3.11] gh-114763: Protect lazy loading modules from attribute access …
46f821d 
…races (GH-114781) (GH-115871)

gh-114763: Protect lazy loading modules from attribute access races (GH-114781)

Setting the __class__ attribute of a lazy-loading module to ModuleType enables other threads to attempt to access attributes before the loading is complete. Now that is protected by a lock.
(cherry picked from commit 200271c)

Co-authored-by: Chris Markiewicz <[email protected]>
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
@effigies @woodruffw
pythongh-114763: Protect lazy loading modules from attribute access r…
d84c56e 
…aces (pythonGH-114781)

Setting the __class__ attribute of a lazy-loading module to ModuleType enables other threads to attempt to access attributes before the loading is complete. Now that is protected by a lock.
@rokm rokm mentioned this pull request Mar 23, 2024
3.13.0a5 broke importlib.util.LazyLoader #117178
Closed
@hugovk

hugovk commented Mar 23, 2024

Copy link
Copy Markdown
Member

Please see #117178 for a potential regression in 3.11 - 3.13.
effigies 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.
@aisk aisk mentioned this pull request Mar 23, 2024
gh-117178: Recover lazy loading of self-referential modules #117179
Merged
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@effigies @diegorusso
pythongh-114763: Protect lazy loading modules from attribute access r…
31c864c 
…aces (pythonGH-114781)

Setting the __class__ attribute of a lazy-loading module to ModuleType enables other threads to attempt to access attributes before the loading is complete. Now that is protected by a lock.
This was referenced Apr 17, 2024
GH-114781 potentially breaks gevent: threading becomes pre-imported at startup #117983
Closed
gh-117182: Allow lazily loaded modules to modify their own __class__ #117185
Merged
LukasWoodtli pushed a commit to LukasWoodtli/cpython that referenced this pull request Jan 22, 2025
@effigies @LukasWoodtli
pythongh-114763: Protect lazy loading modules from attribute access r…
d990899 
…aces (pythonGH-114781)

Setting the __class__ attribute of a lazy-loading module to ModuleType enables other threads to attempt to access attributes before the loading is complete. Now that is protected by a lock.
@emlys emlys mentioned this pull request Jul 28, 2026
Add pygeoprocessing.include_dir() and lazily import the dependency tree natcap/pygeoprocessing#487
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aisk aisk aisk left review comments
@brettcannon brettcannon brettcannon approved these changes
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@ncoghlan ncoghlan Awaiting requested review from ncoghlan ncoghlan is a code owner
@warsaw warsaw Awaiting requested review from warsaw warsaw is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

topic-importlib type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Accessing attributes of a lazily-loaded module is not thread-safe

Uh oh!

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

5 participants

@effigies @brettcannon @bedevere-bot @hugovk @aisk
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.