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-3108: Avoid materializing f_locals for KI protection #3110

Merged
merged 39 commits into from
Oct 27, 2024

Conversation

graingert
Copy link
Member

@graingert graingert commented Oct 13, 2024

Fixes #3108
Fixes #2670

In this pull request, we avoid materializing f_locals by using weak references to code objects instead.

Copy link

codecov bot commented Oct 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.62%. Comparing base (13d4bad) to head (5d76de2).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3110      +/-   ##
==========================================
+ Coverage   99.58%   99.62%   +0.03%     
==========================================
  Files         121      122       +1     
  Lines       18166    18340     +174     
  Branches     3268     3281      +13     
==========================================
+ Hits        18091    18271     +180     
+ Misses         52       47       -5     
+ Partials       23       22       -1     
Files with missing lines Coverage Δ
src/trio/_core/_ki.py 100.00% <100.00%> (ø)
src/trio/_core/_run.py 99.02% <100.00%> (-0.01%) ⬇️
src/trio/_core/_run_context.py 100.00% <100.00%> (ø)
src/trio/_core/_tests/test_ki.py 99.77% <100.00%> (+1.93%) ⬆️
src/trio/_core/_tests/test_run.py 100.00% <100.00%> (ø)
src/trio/_tools/gen_exports.py 95.90% <100.00%> (ø)

@CoolCat467 CoolCat467 changed the title gh-3108: avoid materializing f_locals by using weakrefs to code objec… gh-3108: Avoid directly referencing f_locals Oct 13, 2024
@graingert graingert marked this pull request as ready for review October 13, 2024 20:48
@graingert graingert changed the title gh-3108: Avoid directly referencing f_locals gh-3108: Avoid materializing f_locals for KI protection Oct 13, 2024
@graingert graingert marked this pull request as draft October 14, 2024 06:30
src/trio/_core/_ki.py Outdated Show resolved Hide resolved
@graingert graingert marked this pull request as ready for review October 14, 2024 15:30
Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plenty of nitpicks or me being confused; please don't assume I know what I'm doing with these comments. If the question doesn't make sense, it's cause I don't understand and not some weird encoded question.

src/trio/_core/_ki.py Show resolved Hide resolved
src/trio/_core/_ki.py Outdated Show resolved Hide resolved
src/trio/_core/_ki.py Show resolved Hide resolved
src/trio/_core/_ki.py Outdated Show resolved Hide resolved
newsfragments/3108.bugfix.rst Outdated Show resolved Hide resolved
@graingert
Copy link
Member Author

It looks like this will fix #1752

and maybe in conjunction with #3112 it will fix this #2454

Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bunch of really minor comments. This could merge as is without any changes IMO.

newsfragments/2670.bugfix.rst Show resolved Hide resolved
newsfragments/3108.bugfix.rst Show resolved Hide resolved
src/trio/_core/_ki.py Show resolved Hide resolved
src/trio/_core/_ki.py Show resolved Hide resolved
src/trio/_core/_ki.py Show resolved Hide resolved
src/trio/_core/_run.py Outdated Show resolved Hide resolved
src/trio/_core/_tests/test_ki.py Outdated Show resolved Hide resolved
src/trio/_core/_tests/test_ki.py Show resolved Hide resolved
@A5rocks
Copy link
Contributor

A5rocks commented Oct 24, 2024

Nevermind, I was thinking about whether a deprecation is possible but it isn't because the problem is functions having ki protection only sometimes.

@graingert
Copy link
Member Author

pre-commit.ci autofix

Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be explicit, since all my comments were fixed and all my questions answered.

Copy link
Member

@CoolCat467 CoolCat467 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, this looks pretty great

@graingert graingert enabled auto-merge (squash) October 27, 2024 08:52
@graingert graingert merged commit 57452ad into python-trio:main Oct 27, 2024
39 checks passed
@graingert graingert deleted the ki-with-code-objects branch October 27, 2024 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants