[3.12] gh-58956: Set f_trace on frames with breakpoints after setting a new breakpoint (GH-124454) #125549
Conversation
|
Hi @iritkatriel , in order to make this work, I need to add |
Lib/bdb.py
Outdated
| @@ -84,6 +85,9 @@ def trace_dispatch(self, frame, event, arg): | |||
|
|
|||
| The arg parameter depends on the previous event. | |||
| """ | |||
|
|
|||
| self.enterframe = frame | |||
There was a problem hiding this comment.
does this change the lifetime of frame?
There was a problem hiding this comment.
Ah, interesting observation. Yes this holds a reference to frame (but only one) and could potentially extend the lifetime of it. The existing code in main has this issue as well. Not sure how serious this is - it's not ideal, but it will only hold reference to one frame. We could do a weakref here (but we need to do self.enterframe() because it's a ref now and that's not great), or we could reset it somewhere (but we need to change plenty of the structure of the dispatching code), or we could just not patch this in 3.12.
There was a problem hiding this comment.
You hold a ref to one frame, but this frame links to all the others throughout f_back. It can be a lot.
What's the scenario where this is the only reference to this frame?
There was a problem hiding this comment.
When the program runs but bdb is left there - because we never clear this. There are other frames that we keep holding in bdb like self.botframe and self.stopframe. One solution could be clear all of them in set_continue when there's no breakpoint - so if the user continues the program without debugger overhead, all the frame references will be released. I think that might worth a separate PR.
|
Hey @iritkatriel , we need to determine the fate of this PR because this is a backport. This might result in frames that are not released in time, but this only happens when users debug the program, so production code is not impacted. Maybe this could be a reason for this to be merged? If this is indeed a concern, I can make a PR to clear all the frame references in continue. |
|
At the end of the day it's up to the release manager. This PR is not a clean bug fix, and this discussion actually hi lighted that there's a regression in 3.13 as well -- potentially leaking an unlimited number of frames is something that needs to be fixed. |
The user would need to create unlimited number of |
|
Let's bring @Yhg1s into the discussion as the RM. Summary of the situation: We need to fix a pdb bug in 3.12, but the fix will introduce a reference to a frame object even after the user do I plan to fix this leak in main, but whether to backport is worth a discussion. We have the following solutions:
3.13 already has the frame leak issue, do you want it to be fixed as a bug or leave it as it is? Sorry @hugovk I thought you are the RM of 3.13 :) accidentally ping. |
|
The leak itself isn't really the issue, but the debugger keeping the frame around (and all parent frames, and all objects referenced from each frame's stack) much longer than they would otherwise be is rather unfortunate, and can make certain issues much harder to debug. Given that the bug being fixed here is quite old, I think it's safer to wait with backporting the fix until the change in behaviour can be avoided. |
|
Ping. What's the status of this PR? |
|
Ah, I will take care of it this week. |
…a new breakpoint (pythonGH-124454) (cherry picked from commit 12eaadc) Co-authored-by: Tian Gao <gaogaotiantian@hotmail.com>
041cc02 to
ea13885
Compare
|
Finally this works @iritkatriel . With the latest changes merged to 3.12, this bug fix has no ref leak issue anymore. I think we can merge it now. |
|
Hey @iritkatriel , in case it's not clear in my previous message, I was asking for a final review :) |
| # and set f_trace to trace_dispatch if there could be a breakpoint in | ||
| # that frame. | ||
| frame = self.enterframe | ||
| while frame: |
There was a problem hiding this comment.
| while frame: | |
| while frame is not None: |
There was a problem hiding this comment.
Should we just keep the consistency because it was backported?
(cherry picked from commit 12eaadc)
Co-authored-by: Tian Gao gaogaotiantian@hotmail.com