Make sure the QApplication doesn't get garbage collected

[jasperges]

Following up on this 'bug', reported on the Avalon Gitter.

The show() function for the tools creates a QApplication if needed (via tools.lib.application()). The problem however is that the QApplication created in this way will possibly be garbage collected which in turn causes issues with for example module.window.close().

This makes this 'convenience helper' not so convenient anymore. I think we should fix it, so the QApplication will never be garbage collected. Else it doesn't serve it's purpose and an integration is better off creating the QApplication by itself.

[BigRoy]

I believe it should never get garbage collected by default as it creates a singleton internally? That's also why you can retrieve it with QApplication.instance() right? (Edit: I see this actually came up in the link too)

Anyway, I wonder why this hasn't been a problem for any other hosts (even those without Qt). Am I missing something obvious?

[jasperges]

To be honest: I have no idea if it should or should not get garbage collected. The fact is, it somehow was. Or to be precise: the underlying C++ object was gone.
Would it hurt to make sure a reference is kept to the QApplication?

Have there been any other hosts (apart from Blender) who don't have Qt? For the Blender integration it isn't a problem, because it creates a QApplication by itself, so it doesn't rely on the code in tools.lib.application.

[tokejepsen]

I can confirm that I'm getting the same issue with the Photoshop integration.

Every second opening of any of the tools, will crash the web server without any error messages.

For example commenting out https://github.com/getavalon/core/blob/master/avalon/tools/contextmanager/app.py#L218-L222 will allow me to open the context app more than once.

[BigRoy]

For example commenting out https://github.com/getavalon/core/blob/master/avalon/tools/contextmanager/app.py#L218-L222 will allow me to open the context app more than once.

Actually, isn't that the opposite behavior? This actually sounds logical to me. Whenever the last Qt application's window closes I believe by default the QApplication instance is supposed to "close off" which likely leaves the Window object in an invalid state. Basically meaning the Window would be destroyed meaning whatever is stored in module.window is basically a dangling pointer to nothingness. Making this crash resolved with a simple "check" whether QWindow is still valid.

Does adding this help?

if module.window:

It seems in the current code we're trying to close a QWindow that doesn't actually exist anymore.

I'm not sure that's the valid way to check whether the QObject still exists in memory, but I'm expecting it would be. This however hints that it could be more complicated than just the if statement.

@tokejepsen the other way around is to force the QApplication to persist even after closing the last window:

app.setQuitOnLastWindowClosed(False)

Does that also fix the crash?

[tokejepsen]

Whenever the last Qt application's window closes I believe by default the QApplication instance is supposed to "close off" which likely leaves the Window object in an invalid state.

Yeah, you are right!

Does adding this help?

The module.window point to an invalid window, but I'm not sure how to figure out whether its valid or not, cause its the QApplication instance that is gone and not the window instance.
Will investigate this.

[tokejepsen]

This issue can be solved by introducing an existing_app variable on the lib module. Then we can query whether its a new application or an existing one.

But I would also like to figure out, why do we need to close and delete the existing window in those lines?
I tried to show the context manager multiple times in Nuke, and didnt get any duplicated windows.

[tokejepsen]

I tried to show the context manager multiple times in Nuke, and didnt get any duplicated windows.

Tried in Maya, and its seems like it does make duplicate windows there, so will need those lines of code. Not sure why it does not make duplicates in Nuke.

[mottosso]

Does holding onto the instance of a QApplication solve this issue? What if it's being held onto at the module-level, like tools are, from the lib.application() call?

core/avalon/tools/lib.py

Lines 21 to 31 in ace0c79

	def application():
	app = QtWidgets.QApplication.instance()
	if not app:
	print("Starting new QApplication..")
	app = QtWidgets.QApplication(sys.argv)
	yield app
	app.exec_()
	else:
	print("Using existing QApplication..")
	yield app

E.g.

def application():
  app = module._app or QApplication()

  ...

  module._app = app

With an underscore, to prevent accessing it from the outside.

[tokejepsen]

Does holding onto the instance of a QApplication solve this issue? What if it's being held onto at the module-level, like tools are, from the lib.application() call?

I have tried to store the QApplication on the module level, but on the second try of launching the tool becomes unresponsive and I dont get why that is?

def show(parent=None):
    try:
        module.window.show()
    except (RuntimeError, AttributeError):
        # RuntimeError if the module.window C++ object has been deleted.
        # AttributeError if the module.window is None.
        with lib.application() as app:
            window = Window(parent)
            window.show()
            window.setStyleSheet(style.load_stylesheet())

            module.window = window
            module.qapp = app

            window.activateWindow()

This is tested with the upcoming webserver, which is the base for the Photoshop integration. Its running the tools standalone, similar to how the TV Paint integration would which is the original issue here.

[mottosso]

I would take a step back. Ensure you can get a QPushButton running first. Off the top of my head, it looks like the QApplication isn't running on that second try. What you're seeing is what would happen if you didn't start one to begin with.

[tokejepsen]

Ok, I seem to be able to hold onto the QApplication at the module level but when holding onto the window then I get the above unresponsive dialog that takes the webserver down with it.