Catalogue : Fix duplicate save processes when IPR ends

[johnhaddon]

This bug was introduced in d67ace3. What I'd failed to realise at the time was that Display::driverClosed() will return True before Display::imageReceivedSignal() has been emitted on the UI thread, because it's just an accessor to an atomic flag. This means that its the first call to InternalImage::driverClosed() that will initiate the saving process, rather than the last call as before. So in subsequent calls we need to check we haven't yet initialised m_saver to avoid repeating the save process unnecessarily.

For people with a lot of AOVs, the slowdown from the multiple saves could be quite drastic. What made it even worse was that the assignment of m_saver = newRedundantAsynchronousSaver would block, because the destructor for the old saver waits for the background process to finish.

[danieldresser-ie]

The code change is simple, and makes perfect sense.

The only question is that failing test ... I'm just guessing here, but: we start a thread for all the drivers ourselves, and then call closingThread.join() to make sure that they run before we call handler.assertCalled() for each driver, which works on Windows. We then expect one more UI call from the saver thread - but we're not doing a join() on the saver thread to make sure it actually runs ... waiting 30 seconds should be plenty for it to run ... but maybe something about CI having limited threads to work with? You said you tested on Windows, do you have a way to emulate the thread limitations imposed by CI?

I dunno, this is all just guesswork, but I guess on the positive side, my guesswork is maybe leaning towards it potentially being something wrong with the test rather than something that affects users.

Have you done a basic interactive test on Windows to make sure it doesn't explode in practice?

[johnhaddon]

I think I've got to the bottom of the Windows test failure. On the Windows CI machines, the temp dir has a ~ in it (but not at the start), which is enough to make the Catalogue think that the directory plug that specifies the location to save to has variable substitutions. And the Catalogue will only save if it's inside a ScriptNode to provide the variables to use for those substitutions. And my test didn't use a ScriptNode, and the other tests for saving did. So my test never saved the image, hence why assertCalled() failed. For now I've just updated the test to use a ScriptNode, but it might be worth making the Catalogue a bit less finicky about this.