Fix #2375 but without timing dependent tests

[haikusw]

Last fix for #2375 was not a fix. Broke multiple copy running test (on macOS at least), despite all tests passing.

---

Reworked this fix and added at least partial coverage for clearing the shared memory on exceptions. Updated concurrent version tests, and added new test for exception handler clearing of shared memory on exception before exiting.

Basic idea was to add a SharedMemoryMutex.release() method that can be called before app exit to clean up the shared memory attachment done by this instance. Then call that method before calling exit and for exception exits (crash).

Implementation in app.py:

• add the release() method to SharedMemoryMutex.
• update the excepthook implementation to call _shared_memory.release() before calling sys.exit.
• remove the incorrect _shared_memory.detach() call in exit.
• update the end of run(): to call appexec(), save the exit_status, call _shared_memory.release(), and then call sys.exit with the saved exit_status.

Then I had to modify the explicit sys.exit(0) in logic.py on line 1431 so that it wouldn't short circuit exit the app and cause appexec() to never return and thus our exit clean up of the shared memory not get called.

Changing logic.py line 1431 from sys.exit(0) to QtCore.QCoreApplication.exit(0) results in app_exec_() returning and the cleanup code I added in app.py run(): to call the new _shared_memory.release() method and this appears to fix the issue for real for the clean no-error exit path.

I did have to rework test_logic.py's test_quit_calls_sys_exit(mocked_session) to expect the right thing instead of sys.exit explicitly.

Then I modified the existing tests in app.py:

• test_only_running_once() to clean up after itself by calling _shared_memory.release() explicitly since it is a focused test that doesn't run the app event loop.
• similarly test_running_twice() had to be modified to also have the first run process clean up after itself when it exits.
• Finally, I added a new two new tests to test_app.py test the exception shared memory cleanup handling, test_running_twice_after_exception() and test_running_twice_after_generic_exception().

---

Caveats - there may be other places where sys.exit is being called that short circuits the app_exec() event loop and prevents the shared memory cleanup code from getting called. That will be no worse than the master was before I tried to fix this the first time, but it would be good to track those down and convert them to QtCore.QCoreApplicaton.exit or QApplication.exit, as appropriate for the scope they are in.

---

Sorry this got a bit more complicated, but I believe it's a more complete fix now since it adds at least some of the exception path cleanup whereas previously it did not.

---

I rewrote the test_running_twice() test because it was flakey due to using sleep(x) making it very sensitive to execution environment (much slower on the Pi tests runners, for example). It now uses chaining of process launches.

There is a comment that says this approach will not work (and in discussion I believe Windows was brought up as the problem platform). However, it is passing all of CI here and this comment is about the test itself and not the application code, so I think it's ok.

[haikusw]

Test PiOS buster-legacy seems to be a runner problem.

I don't understand why the other tests are "queued" but not running...

[haikusw]

Does look like all the Windows tests passed so maybe that comment was incorrect? Or is not longer applicable with the current code?

[haikusw]

@tjguk any idea why 9 checks are queued but then never run? Second time now that it's done that so something seems broken but I don't understand what.

[carlosperate]

I've updated the runners and merge to the main branch, if you rebase or merge it should resolve the macos-15 and ubuntu-18.04 failures.

[haikusw]

I've updated the runners and merge to the main branch, if you rebase or merge it should resolve the macos-15 and ubuntu-18.04 failures.

Thanks! I was just working through a patch to do that as I dug deeper into why :)

[haikusw]

@carlosperate Looks like this all passed except for the same virtualenv test that failed on your CI test for the updated runners (seems like it's time sensitive and that's always iffy on CI in my experience (hence this PR in fact)).

It would be great if someone could test this branch on a Windows computer because I vaguely recall that that is where the comment applied:

# It's important that the two competing processes are not part of the same
# process tree; otherwise the second attempt to acquire the mutex will
# succeed (which we don't want to happen for our purposes)

CI is not currently showing that this is an issue, but it would be good to have someone test to make sure that launching a second copy of Mu when one is running just causes that second copy to quit silently.

---

EDIT: Actually, this only impacts the test (not running the app) and tests all ran and passed (other than an unrelated flakey time-sensitive virtualenv test), so really I think this is ready for review and merge.

[haikusw]

Added the relevant comments from #2386 to the first comment of this PR, but there are some longer term improvement things that are in the #2386 PR thread if anyone is wanting to improve this further. I've put in what I can at this point so hopefully it meets the goals of @tjguk's original goals more fully now and works everywhere as intended.

[haikusw]

@ntoll you asked if this fix was ready some months ago and I was not able to get it ready then (apologies). I'm back semi-functional again and I have done my best to fix and improve this to the point that it's ready. It's ready for review.

[tjguk]

Thanks for all this work, @haikusw -- I'm really hoping to be able to review over this weekend. (Not sure where you're based, but there's a public holiday here in the UK tomorrow). This is definitely on my to-do list

[haikusw]

Cool. Thanks. I’m in Oregon, USA.

Sent from my phone

On May 6, 2023, at 11:40 PM, Tim Golden @.***> wrote:

 Thanks for all this work, @haikusw -- I'm really hoping to be able to review over this weekend. (Not sure where you're based, but there's a public holiday here in the UK tomorrow). This is definitely on my to-do list

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[haikusw]

I’d love to add technical docs for this, and a CI scanner to catch new additions of sys_exit, but current health challenges mean I’m not able to do this (sorry).

[tjguk]

Ok -- many thanks to you @haikusw. I'm going to merge this. I've reviewed the approach and run it through on Linux & Windows. And all the GH CI tests pass. After the merge I'll watch the GH actions in case of any surprises.

But basically I think we're good to go

[haikusw]

Thank you for making the time to review this. I appreciate it and I’m glad that you found it to be ready. If any questions come up about it, feel free to reach out and I’ll do what I can.