Fix #514

[tokejepsen]

What's changed?
Store QApplication at module level and always exec.

Spawned from #520

This has been tested standalone and in Maya/Nuke.

[BigRoy]

In essence this would still be wrong - there's no need to store the QApplication. Wasn't that what we got from the discussion and looking into the issue?

The QApplication.instance() method will always return the singleton instance, however in some cases just exec_() had to trigger again which you're now doing.

So wouldn't you only need to move the call to app.exec_()?

[tokejepsen]

Wasn't that what we got from the discussion and looking into the issue?

What I'm trying to fix here is the segfault that happens when using the the tools standalone, which I would imagine is the same issue as described in #514
I was not able to hold onto the QApplication on a tool level successfully, so this is the solution.

[BigRoy]

What I'm trying to fix here is the segfault that happens when using the the tools standalone, which I would imagine is the same issue as described in #514
I was not able to hold onto the QApplication on a tool level successfully, so this is the solution.

What's the easiest way for me to reproduce this segfault on my end? Can I reproduce this in standalone Python?

Any python Qt library should always cling on to the actual QApplication instance - it should somehow keep that reference unless maybe you'd reload the Qt libraries. :) I still feel, as I described there, that the actual segfault happens due to accessing the closed Qt widget with module.window for a Window that is actually at that point in time on a quited/closed QApplication. Meaning the problem is not on the QApplication but on the Window widget. Sorry for being so pushy on this. I'd just like to figure out why exactly this is happening.

[BigRoy]

@tokejepsen does this behave the same?

Run just in a Python command line interpreter.

# assumes a running QApplication instance
from PyQt5 import QtWidgets, QtCore

app = QtWidgets.QApplication([])
        
window = QtWidgets.QDialog()
window.show()

app.exec_()

# this will crash, similar to what we're doing with `module.window`
window.show()

# note that this line below isn't even reached...
# because it crashes on the window.show()
# in this example both `app` and `window` variable persist
# but the crash is due to window having been destroyed...
app.exec_()

1. Window works fine.
2. Close the window.
3. This closes down the QApplication (but instance persists - also because it's a variable)
4. Show the closed window... that is a destroyed/invalid QObject now due to QApplication shutting down.

This seems like the problem you're running into instead.

Maybe we should add on destroyed callback that removes itself from module.window and setting it back to None to ever avoid accessing the dangling pointer to the destroyed window? That would be the only safe way to remove the dangling reference, according to this: https://stackoverflow.com/a/32218542/1838864

[tokejepsen]

does this behave the same?

Tried with "your" code below and got the window to how twice but the python process is stuck.

import sys
import contextlib
# assumes a running QApplication instance
from avalon.vendor.Qt import QtWidgets


@contextlib.contextmanager
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


with application():
    app = QtWidgets.QApplication([])

    window = QtWidgets.QDialog()
    window.show()

    app.exec_()

    window.show()

    # note that this line below isn't even reached...
    # because it crashes on the window.show()
    # in this example both `app` and `window` variable persist
    # but the crash is due to window having been destroyed...
    app.exec_()

This is not what happens when I see the segfault. The python process crashes and exits.

What's the easiest way for me to reproduce this segfault on my end? Can I reproduce this in standalone Python?

I honestly dont know how and this is clearly going way over my head and comprehension. All I can say it that it makes the Photoshop integration which I'm working on, not error out, without affecting Nuke/Maya.

Maybe the title is wrong and I shouldn't assume it'll fix #514.

[BigRoy]

Funny that you say it now crashes on you too. I'm trying it again now... and it works fine. Ha, what is going on? It's as if the Window doesn't get destroyed/garbage collected in time one some occassions and it remains valid then?

Anyway... can we add a comment above the line where store the QApplication into self.app as to why we're doing it, and potentially mentioning #514 so it's clear as to why it's there. If it helps you to continue we can always investigate later as to what exactly is going on.

@jasperges could you see whether this fixes your issues too? Specifically #514 ?

[tokejepsen]

I have noticed just now that the Loader tool handles an existing module window badly; https://github.com/getavalon/core/blob/master/avalon/tools/loader/app.py#L389-L408

This causes the Loader to become unresponsive on second showing.

[jasperges]

@tokejepsen Please, don't always run self.app.exec_(). This screws up the Blender integration. I also think it's the responsibility of the creator of the QApplication to use exec_() if needed and shouldn't be called (again) for an existing QApplication instance.

If you could move it to the else block, I would be very happy. 🙂

[tokejepsen]

This is one of the issues that I'm seeing with the Photoshop integration. If we dont have that exec_() then I get an unresponsive window on the second launch.
Again not sure what the issue is with the unresponsive window.

What happens in Blender with always running exec_()?

[BigRoy]

This is one of the issues that I'm seeing with the Photoshop integration. If we dont have that exec_() then I get an unresponsive window on the second launch.
Again not sure what the issue is with the unresponsive window.

The unresponsive window should be QApplication stopping with executing. This happens by default when the last Qt window closes as described at the top of this comment.

---

@jasperges would love to understand too why ._exec() would fail in Blender. From what I understood so far from Toke/David it will just return -1 in that case and disregard it.

[jasperges]

.exec_() doesn't fail, but it makes Blender unresponsive. After you quit all Qt Windows, Blenders GUI is responsive again.

Instead I use QApplication.processEvents() (which gets triggered every few milliseconds) to keep the Qt windows 'alive'.

[BigRoy]

The problem that @jasperges describes should also be happening in C4D. In both those applications you should never trigger QApplication.exec_().

Should we maybe push those behind an environment variable for those applications that should never trigger it? AVALON_QTAPPLICATION_EXEC = False

Potentially adding it in the host's install() function?

[tokejepsen]

Should we maybe push those behind an environment variable for those applications that should never trigger it? AVALON_QTAPPLICATION_EXEC = False

Sounds good to me.

[jasperges]

Fine for me. One question though: why should the previous behaviour need to be changed at all?

If a new QApplication is spawned, run QApplication.exec_(). If an existing QApplication is already running, you might expect that it's event loop is also already running, right?

So apart from it causing issues with running Blender and Cinema4D (and possibly others), I don't understand why QApplication.exec_() should always be triggered.

[tokejepsen]

@jasperges see #523 (comment)

[jasperges]

@jasperges could you see whether this fixes your issues too? Specifically #514 ?

@BigRoy I guess so, I will have to check with @kguyaux and his TVPaint integration.

[tokejepsen]

Have put the execution of existing QApplications behind the environment variable AVALON_QTAPPLICATION_EXEC.
How do we feel about this PR?
This is still needed for #524

[BigRoy]

I think the change made in this commit with the environment variable actually also would be intended for the first start of the QApplication. If I'm not mistaken, then for Blender and Cinema4D you should never call .exec_() as it holds the thread and will freeze the host application. Right @jasperges ? It's super weird.

Previously I worked around it by creating an Instance before calling any of the Avalon tools as that would mean it would always skip trying to call exec_(). Maybe it makes sense to place the environment variable for first creation of QApplication too so the "hack" isn't needed?

---

Other than that, I can live with this PR. It's one of those oddities I don't really understand but it seems to fix things for your use case so I guess it's a required change.

[tokejepsen]

Previously I worked around it by creating an Instance before calling any of the Avalon tools as that would mean it would always skip trying to call exec_(). Maybe it makes sense to place the environment variable for first creation of QApplication too so the "hack" isn't needed?

Hmm, I think I should maybe follow your example and just enforce the QApplication for the Photoshop integration only.

This PR does not have enough grounding, and it affects too much code to just add. Closing this and fixing in #524