You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
test_frame fails when running with -R 3:3 argument #116098
Okay it took me a while, but I don't believe this is a "bug" the commit brings - more like a very deligate(fragile) test is broken because it depends on some undocumented(undefined) behavior.
I believe @brandtbucher is the author for the test. If I understand it correctly, the test requires a gc triggers in trace function - which is not guaranteed. Normally RESUME and INSTRUMENTED_RESUME triggers the gc, but RESUME_CHECK does not. I think it's intentional @markshannon ? The commit mentioned (correctly?) converts the RESUME instruction for trace to RESUME_CHECK which is not able to trigger gc anymore, thus fails the test.
So we need to confirm a couple of things before fixing this:
Should we fix the test, or the code?
Is it reasonable for the test to require a gc in trace?
Is it intentional that RESUME_CHECK does not trigger gc? Is that okay?
Is it valid to have RESUME_CHECK for trace instead of RESUME or INSTRUMENTED_RESUME?
If we need further discussion for this and can't come up with a fix today, I'll revert this commit by the end of today and wait for the final decision.
Thank you for the revert.
Sorry that it's your commit that got caught here. Reverting is a way to get the buildbots green so they can catch more issues, not a judgement of your work.
I understand - it's important to keep the test clean otherwise the test lost its meaning. I'll work on the fix and hopefully we can come up with something and check it back in.
Activity
Eclips4 commentedon Feb 29, 2024
Bisected to 0a61e23
cc @gaogaotiantian
gaogaotiantian commentedon Feb 29, 2024
Okay it took me a while, but I don't believe this is a "bug" the commit brings - more like a very deligate(fragile) test is broken because it depends on some undocumented(undefined) behavior.
I believe @brandtbucher is the author for the test. If I understand it correctly, the test requires a gc triggers in
tracefunction - which is not guaranteed. NormallyRESUMEandINSTRUMENTED_RESUMEtriggers the gc, butRESUME_CHECKdoes not. I think it's intentional @markshannon ? The commit mentioned (correctly?) converts theRESUMEinstruction fortracetoRESUME_CHECKwhich is not able to trigger gc anymore, thus fails the test.So we need to confirm a couple of things before fixing this:
trace?RESUME_CHECKdoes not trigger gc? Is that okay?RESUME_CHECKfortraceinstead ofRESUMEorINSTRUMENTED_RESUME?gaogaotiantian commentedon Feb 29, 2024
If we need further discussion for this and can't come up with a fix today, I'll revert this commit by the end of today and wait for the final decision.
gh-116098: Revert "gh-107674: Improve performance of `sys.settrace` (G…
encukou commentedon Mar 1, 2024
Thank you for the revert.
Sorry that it's your commit that got caught here. Reverting is a way to get the buildbots green so they can catch more issues, not a judgement of your work.
gaogaotiantian commentedon Mar 1, 2024
I understand - it's important to keep the test clean otherwise the test lost its meaning. I'll work on the fix and hopefully we can come up with something and check it back in.
pythongh-116098: Revert "pythongh-107674: Improve performance of `sys…
pythonGH-116098: Remove dead frame object creation code (pythonGH-116687
pythonGH-116098: Remove dead frame object creation code (pythonGH-116687
pythongh-116098: Revert "pythongh-107674: Improve performance of `sys…
pythonGH-116098: Remove dead frame object creation code (pythonGH-116687
pythongh-116098: Revert "pythongh-107674: Improve performance of `sys…
pythonGH-116098: Remove dead frame object creation code (pythonGH-116687
Eclips4 commentedon May 8, 2024
@gaogaotiantian can it be closed now?
gaogaotiantian commentedon May 8, 2024
Yes. The root cause was that the test is not valid anymore and we removed the test.