wsd: clean up the coolwsd process cleanup

[Ashod]

Simplifies the cleanup process at exit and ensures that cleanup() is always called.

[stbergmann]

cleanup can now be called multiple times; that doesn't smell like a good design

[Ashod]

I suppose if it throws. I moved it out, so if it throws that ends the process promptly.

[stbergmann]

Sorry, I don't get you What I meant was that when the call to cleanup in the try block threw, the catch blocks would have called cleanup a second time. (But you seem to have addressed that in b92c610 now, anyway?)

[Ashod]

Yes, that's what I meant by "if it throws" in the try, we'd call it (cleanup) again it the catch. Yes, addressed it.

[stbergmann]

for IOS, all the above code wasn't originally executed, but now is

[Ashod]

True. But on iOS we never run unit tests. Still, best to exclude them.

[stbergmann]

this is not only about the

        if (UnitBase::isUnitTesting())

but also about e.g. the

        if constexpr (Util::isMobileApp())
            Util::forcedExit(returnValue);

[Ashod]

Yes, understood. Addressed.

[stbergmann]

Ashod force-pushed the private/ash/exit branch from 57cfe33 to b92c610

I find the github PR review interface extremely hostile and hardly usable. Now, it appears that your new commit has a different parent than your old commit, so the "Compare" button leads to a page full of noise. I don't know how to meaningfully review such a PR.

[Ashod]

Ashod force-pushed the private/ash/exit branch from 57cfe33 to b92c610

I find the github PR review interface extremely hostile and hardly usable. Now, it appears that your new commit has a different parent than your old commit, so the "Compare" button leads to a page full of noise. I don't know how to meaningfully review such a PR.

Understood. These are the diffs due to rebasing. I'll try to avoid that in the future.

Meanwhile, I'm hoping the PR is small enough, and your comments few enough that you can look at the parts we discussed. I'm hoping the changes are minor enough that you'll tell me if you still have any concerns.

[stbergmann]

this shouldn't be done for IOS (for which Util::isMobileApp() is true AFAIU)

[stbergmann]

And, on second thought, moving the Util::forcedExit(returnValue); into COOLWSD::cleanup is not nice: if COOLWSD::innerMain throws, we want to experience that (std::terminate), and not get the exception "swallowed" by the catch block around the call to innerMain calling cleanup -> forcedExit.

So, if you want to move the call to UnitBase::uninit() out of COOLWSD::innerMain, then maybe leave the

    if constexpr (Util::isMobileApp())
        Util::forcedExit(EXIT_SUCCESS);

there after all (on the, somewhat dubious, grounds that mobile apps don't run unit tests and thus don't need UnitBase::uninit, which you are already relying on here anyway)

[Ashod]

And, on second thought, moving the Util::forcedExit(returnValue); into COOLWSD::cleanup is not nice: if COOLWSD::innerMain throws, we want to experience that (std::terminate), and not get the exception "swallowed" by the catch block around the call to innerMain calling cleanup -> forcedExit.

Thanks, @stbergmann. This makes sense. I tried to retain the original IOS code while making sure that we do call the unit-test uninit in the case of exceptions.

I rebased, forgetting that it messes the diff. Please take a look at the diff as it is. It is now closer to the original and should be easy to re-review.

Thanks for your detailed reviews. Appreciated.