FIX: maintain reference to related displays to avoid gc #1137
Conversation
| # Parent the display to avoid garbage collection | ||
| display.setParent(self) | ||
| display.setWindowFlags(Qt.Window) | ||
| display.show() |
There was a problem hiding this comment.
There was a problem hiding this comment.
It doesn't appear without the second show, somehow
|
This seems to have a side-effect of causing displays opened by a non pydm-app to close whenever their parent display is closed, which did not happen before. You can try this on one of the lucid screens using your branch vs whatever version of pydm dev_conda uses. Maybe a parent was intentionally not set for this reason? |
This is a good point. Maybe this should be implemented differently. |
I think this is intentional pyqt behavior. widgets without a parent are considered top-level and can exist independently (garbage collection notwithstanding) - widgets with a parent only exist as long as their parent does no matter what window they're in. One alternative is to just add a list member to this class and add any non-pydm opened displays to the list without setting their parent. This stops garbage collection and keeps the new display top-level. |
I'll experiment a bit. My intuition is that stashing these in a list could be problematic in a memory-management sense for displays that are left open for a long time, if the user clicks the button a lot and closes the displays. |
|
It turns out to be pretty simple to prune the list over time to avoid the reference list growing forever. |
ZLLentz
changed the title
|
I'm pretty happy with where this PR ended up last year. I think the only thing left to possibly add would be an extra unit test that runs through this code path and tries to execute the garbage collector, confirming that the subscreen stays open. This might be at least a little bit tricky to write. |
We noticed a niche issue when it comes to using the
PyDMRelatedDisplayButtonwidget outside of a pydm application.In these cases, the display that gets loaded is orphaned, and will eventually get garbage collected because there are no python references to the display object. This isn't a problem inside the pydm applications because in these cases we either start a new process, which retains a reference to the screen, or we replace our main widget with the new widget, so the window retains a reference.
Users have been reporting that their displays would "crash with no error message" and I tracked it down to this.
In this PR, I create a list and stash these related displays in the list. This means they have one python reference, so they aren't closed automatically.