Work around infinite recursion in ToolPalette.resizeEvent()

[zakv]

This PR resolves #27. It takes the approach suggested there of effectively turning off the resize event signal while handling a resize event so as to avoid infinite recursion.

The implementation is a bit different than might be expected. The recursion happens because ToolPalette.resizeEvent() calls ToolPalette._layout_widgets(), which can cause more resize events to be added to the Qt event queue, which in turn causes more calls to resizeEvent(). I'm not aware of any good way to prevent those events from being generated (though I'm not a Qt expert so that could very well be possible). Also, the calls to resizeEvent() execute to completion before it is called again by the event loop. That means that temporarily changing things during the method's execution (such as incrementing/decrementing a recursion level counter or temporarily redefining self.resizeEvent() to avoid recursion) don't work.

The approach taken here is that the first call to resizeEvent() sets self._layout_widgets_during_resizeEvent to False to signal that the upcoming calls of resizeEvent() shouldn't call _layout_widgets(). It then calls _layout_widgets() which may cause resize events to be added to the events queue. Finally before returning, resizeEvent() puts a custom event on the end of the event queue which will cause the ToolPalette to set self._layout_widgets_during_resizeEvent back to True again. That ensures that eventually resize events will cause _layout_widgets() again, which is required for resizing to work well. That custom event isn't handled until all of the resize events generated before it in _layout_widgets() run though, which circumvents the infinite recursion.

I've tested this PR with two problematic blacs settings files (including the one mentioned here) and both crashed without the changes here but started up fine with the changes. I've also seen that once blacs is started, the ToolPalettes still seem to resize just fine; the number of widgets per row still increases/decreases as needed when the tab is expanded/contracted.

[zakv]

There are still a decent number of commented-out python-2-style debugging print statements in toolpalette.py which seem to be oriented towards debugging this issue. I could add a commit to this PR to remove them if desired.

[chrisjbillington]

Thanks so much for looking into this! Glad to hear the recursion can be prevented.

A tricky solution, I wonder if something simpler would work. Does a simple counter to prevent recursion work, i.e. the following (untested)? If so, that might be preferable.

Feel absolutely free to delete dead code adjacent to the edits you're actually making, if you want to clean-up code elsewhere that's also fine, but generally better as a separate commit!

--- a/labscript_utils/qtwidgets/toolpalette.py
+++ b/labscript_utils/qtwidgets/toolpalette.py
@@ -264,6 +264,8 @@ class ToolPalette(QScrollArea):
         # allocated rows/columns with columnCount()/RowCount() rather than the number of visible ones
         self._column_count = 0
         self._row_count = 0
+        self._resize_recursion_depth = 0
+        self.MAX_RESIZE_RECURSION = 5

     def addWidget(self,widget,force_relayout=True):
         # Append to end of tool pallete
@@ -386,24 +388,25 @@ class ToolPalette(QScrollArea):
         return QSize(width, height)

     def resizeEvent(self, event):
-        # overwrite the resize event!
-        # print '--------- %s'%self._name
-        # print self._widget_list[0].size()
-        # print self._widget_list[0].sizeHint()
-        # print self._widget_list[0].minimumSizeHint()
-        # print self._layout.rowMinimumHeight(0)
-        # print self.size()
-        # print self.minimumSize()
-        # print self.sizeHint()
-        # print self.minimumSizeHint()
-        #pass resize event on to qwidget
-        # call layout()
-        QWidget.resizeEvent(self, event)
-        size = event.size()
-        if size.width() == self.size().width() and size.height() == self.size().height():
-            # print 'relaying out widgets'
-            self._layout_widgets()
-
+        # This method can end up undergoing infinite recursion for some window layouts,
+        # see https://github.com/labscript-suite/labscript-utils/issues/27. It seems
+        # that sometimes self._layout_widgets() increases the number of columns, which
+        # then triggers a resizeEvent, which then calls self._layout_widgets() which
+        # then decreases the number of columns to its previous value, which triggers a
+        # resizeEvent, and so on. That loop may occur e.g. if increasing/decreasing the
+        # number of columns add/removes a scrollbar, which then changes the number of
+        # widgets that can fit in a row. Limit the recursion depth to
+        # self.MAX_RESIZE_RECURSION to avoid this
+        if self._resize_recursion_depth > self.MAX_RESIZE_RECURSION:
+            return
+        self._resize_recursion_depth += 1
+        try:
+            QWidget.resizeEvent(self, event)
+            size = event.size()
+            if size.width() == self.size().width() and size.height() == self.size().height():
+                self._layout_widgets()
+        finally:
+            self._resize_recursion_depth -= 1

 # A simple test!
 if __name__ == '__main__':

[chrisjbillington]

It sounds like you have thought about this a bit though, and your solution is still quite simple, so if you think it's the right way to go then I'll believe you!

[chrisjbillington]

Another thought, does moving QWidget.resizeEvent(self, event) to be after the code calling self._layout_widgets() make a difference?

[zakv]

Yeah your suggestion with the recursion counter was the first thing I tried and I was very confused when it didn't work. The recursion counter would work for something like this:

MAX_RECURSION = 3
recursion_level = 0

def f():
    global recursion_level
    print(f"Recursion level: {recursion_level}")
    recursion_level += 1
    if recursion_level < MAX_RECURSION:
        f()
    recursion_level -= 1

f()

Which prints what you'd expect:

Recursion level: 0
Recursion level: 1
Recursion level: 2

However, the recursion here works a little differently. It's more like the following:

from queue import Queue

MAX_RECURSION = 3
recursion_level = 0
event_queue = Queue()

def f():
    global recursion_level
    print(f"Recursion level: {recursion_level}")
    recursion_level += 1
    if recursion_level < MAX_RECURSION:
        event_queue.put(f)
    recursion_level -= 1

event_queue.put(f)  # Put in one event to get things started.
while not event_queue.empty():
    next_event_function = event_queue.get()
    next_event_function()

Which just prints the following over and over:

Recursion level: 0
Recursion level: 0
Recursion level: 0
...

The issue with the queue-based recursion is that each call to f() runs to completion before the following recursive call to f() starts. That means that the recursion_level -= 1 line of one call to f() runs before the next call to f() starts, and so recursion_level just goes back and forth between 0 and 1 instead of increasing.

I suspect that the reason blacs crashes is that the equivalent of f() there actually sends multiple resize events to the event queue, and so the event queue grows in each recursive iteration until there's a stack overflow. I haven't explicitly checked that though.

I'll try your other suggestion, namely switching the order of QWidget.resizeEvent(self, event) and self._layout_widgets() and see how it goes. If I understand the issue correctly though I think that with either ordering more resize events get added to the event queue and so the queue never empties.

[zakv]

I just tried switching the ordering of QWidget.resizeEvent(self, event) and self._layout_widgets() then opening blacs with one of the problematic hdf5 settings files (without the changes from this PR). It failed to open with either ordering of those commands

[chrisjbillington]

Thanks Zak. This is different to what I expected, because a stack overflow specifically implies some function is recursing by calling itself, not the queued-event infinite loop you're describing.

But it's true that events are queued - it's only signals that are called immediately with all the re-entrancy that implies. So since we're looking at events and not signals, it makes sense our functions aren't recursing

Perhaps the queue is implemented with actual recursion on the stack somewhere in the Qt internals even though our functions are not re-entering themselves, resulting in the stack overflow that was observed.

Anyway, glad to have it fixed! Thanks again.

[zakv]

Ah gotcha. I haven't done a whole lot of work in low-level languages so my understanding of stack/heap isn't all that great. From this method docsting I thought that events sometimes got allocated on the stack but I'm very likely just misunderstanding that. Your explanation about the queue possibly using recursion internally sounds more reasonable.

Anyway thanks for the code review and the merge!