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

owpreprocess: fix potential crash at destruction #6596

Merged
merged 1 commit into from
Nov 3, 2023

Conversation

markotoplak
Copy link
Member

Issue

When signals are handled when objects are being destroyed we can bugs as in #6136.

The test in https://github.com/biolab/orange3-single-cell/actions/runs/6010124237/job/16300932397?pr=395 failed with

 test_editor_normalize_groups (tests.test_owscpreprocess.TestNormalizeEditor) ... Traceback (most recent call last):
  File "/Users/runner/work/orange3-single-cell/orange3-single-cell/.tox/orange-oldest/lib/python3.9/site-packages/Orange/widgets/data/owpreprocess.py", line 1412, in __update_size_constraint
    sh = self.flow_view.minimumSizeHint()
AttributeError: 'OWscPreprocess' object has no attribute 'flow_view'

Thanks to @elapraznik for the crash. :)

Description of changes

Check if the object is still live.

Includes
  • Code changes
  • Tests
  • Documentation

@janezd
Copy link
Contributor

janezd commented Oct 13, 2023

Zombie objects are bug in PyQt's architecture.

Methods (except those connected to the signal destroyed) shouldn't have to check whether an attribute that is set in the constructor still exist. Hence, I would prefer that this bug is fixed in the meta class: any method that can be called on a zombie (to my and Marko's knowledge: eventFilter) should be wrapped in a method that checks whether self.__dict__ is non-empty. If non-empty, it calls the actual method, otherwise it calls just the super.

@ales-erjavec, would this be OK?

Edit: something like biolab/orange-widget-base#256. This however affects only OWPreprocess and OWBoxplot. All other eventFilters are in classes outside the OWWidget hierarchy.

@ales-erjavec
Copy link
Contributor

I think this would be a better solution

diff --git a/Orange/widgets/data/owpreprocess.py b/Orange/widgets/data/owpreprocess.py
index 0d1c5de8f..872cfcd50 100644
--- a/Orange/widgets/data/owpreprocess.py
+++ b/Orange/widgets/data/owpreprocess.py
@@ -1178,6 +1178,10 @@ class OWPreprocess(widget.OWWidget, openclass=True):
 
         gui.auto_apply(self.buttonsArea, self, "autocommit")
 
+        self.__update_size_constraint_timer = QTimer(
+            self, singleShot=True, interval=0,
+        )
+        self.__update_size_constraint_timer.timeout.connect(self.__update_size_constraint)
         self._initialize()
 
     def _initialize(self):
@@ -1367,8 +1371,7 @@ class OWPreprocess(widget.OWWidget, openclass=True):
 
     def eventFilter(self, receiver, event):
         if receiver is self.flow_view and event.type() == QEvent.LayoutRequest:
-            QTimer.singleShot(0, self.__update_size_constraint)
-
+            self.__update_size_constraint_timer.start()
         return super().eventFilter(receiver, event)
 
     def storeSpecificSettings(self):

@markotoplak markotoplak merged commit 5412e74 into biolab:master Nov 3, 2023
@markotoplak markotoplak deleted the fix-crash-preproc branch November 6, 2023 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants