Question: WinSparkle deadlock if automatic updates enabled and update manually triggered and installed

Hello!

I ran into a race condition / deadlock while using WinSparkle in my Qt application.

I'm assuming I'm doing something wrong, but in the off chance I've found a bug, I wanted to report what I was observing.

The sequence of events seems to be:

Automatic updates are enabled _by previous call to win_sparkle_set_automatic_check_for_updates(1)_
App starts up and calls win_sparkle_init() Winsparkle starts checking for updates in the background
App calls win_sparkle_set_shutdown_request_callback to invoke QCoreApplication::quit
App enters main event loop via app.exec()
User selects "Check for update..." which in turn calls win_sparkle_check_update_with_ui()
Update downloads and user clicks "install"
The installer is launched successfully
WinSparkle proceeds to RequestShutdown https://github.com/vslavik/winsparkle/blob/8a547d6441ffee6a922fb41f88257dab9296c182/src/ui.cpp#L691
App's shutdown request callback is called, QCoreApplication::quit() is invoked
App exits its main event loop
App calls win_sparkle_cleanup() and hangs

Looking at the call stacks I see two threads deadlocked:

// Main UI Thread:

WinSparkle.dll!winsparkle::Thread::Join() Line 155  C++
[Inline Frame] WinSparkle.dll!winsparkle::UIThreadAccess::ShutDownThread() Line 1432    C++
[Inline Frame] WinSparkle.dll!winsparkle::UI::ShutDown() Line 1512  C++
WinSparkle.dll!win_sparkle_cleanup() Line 108   C++

https://github.com/vslavik/winsparkle/blob/8a547d6441ffee6a922fb41f88257dab9296c182/src/threads.cpp#L155

// WinSparkle Updates Check Thread

[Inline Frame] WinSparkle.dll!winsparkle::Event::WaitUntilSignaled(unsigned int) Line 64    C++
WinSparkle.dll!winsparkle::PeriodicUpdateChecker::Run() Line 317    C++
WinSparkle.dll!winsparkle::Thread::ThreadEntryPoint(void * data) Line 123   C++

https://github.com/vslavik/winsparkle/blob/8a547d6441ffee6a922fb41f88257dab9296c182/src/updatechecker.cpp#L316

I'm basically following your "simpler" approach for a Qt app referenced in https://github.com/vslavik/winsparkle/issues/41#issuecomment-70367197 modified with an additional shutdown request callback configured to invoke QCoreApplication::quit().

When RequestShutdown is called, QCoreApplication::exec() returns. The event loop is stopped. And I try to clean up WinSparkle.

I think I'm hanging because the Update Check's m_terminateEvent is never delivered.

To workaround the issue on my end, I can do something like:

 ...
 win_sparkle_init();
 int r = a.exec();
+a.processEvents(QEventLoop::AllEvents, 2*1000);
 win_sparkle_cleanup();
 return r;

Which appears to give's WinSparkle's "Updates Check" thread a chance to notice the termination.

Annoyingly the timeout based overload is required, a few calls to process events are needed to flush everything out

So, a few questions:

Is triggering a manual update while automatic updates is enabled a supported operation?
Should WinSparkle somehow? terminate it's Update Check thread when it knows it's successfully downloaded an update, launched an installer, has shutdown the hosting app, and is apparently on the way out?
Should WinSparkle avoid using the UI thread to send a message after RequestShutdown has been called? https://github.com/vslavik/winsparkle/blob/8a547d6441ffee6a922fb41f88257dab9296c182/src/ui.cpp#L1511 ~I would've thought this was trying to tear down the Update Checks thread, which could be problematic if the apps main event loop is no longer running~

_Edit: Nevermind, my mistake. This wasn't referring to my Qt event loop. SendMsg seems to be calling wxQueueEvent, which is a wrapper around wxEvtHandler::QueueEvent._

_The doc notes it's similar to wxEvtHandler::ProcessEvent (which occurs synchronously). So the asynchronous nature might still be relevant. It notes "this one is asynchronous and returns immediately while the event will be processed at some later time (usually during the next event loop iteration)."_
Or is using a shutdown request callback set to quit the application / terminate the event loop simply a Bad Idea ^TM? And an obvious misuse of the WinSparkle library

Thank you for taking the time to read/respond to my question. I'm still struggling a little to understand exactly where things are going wrong, so if anything I've written is off topic or not relevant, let me know and I can try and simplify the scenario.

Your time / effort maintaining and supporting this library is very much appreciated. If there's more information you need, let me know.

Thanks again, Ryan

_I may also try and see if I can re-create the issue using your sample Qt example application. As best I can tell it should have the same problem if automatic updates are enabled and win_sparkle_cleanup is called after exec() exits_

I'm assuming I'm doing something wrong

Yep. This one is all my fault.

My problem is caused by my code and a faulty can_shutdown_callback implementation that despite it's name apparently results in app.exec() exiting early ahead of RequestShutdown.

The following works without issue:

win_sparkle_init();
win_sparkle_set_shutdown_request_callback([]() {
    QMetaObject::invokeMethod(QCoreApplication::instance(), "quit", Qt::QueuedConnection);
});

exitCode = app.exec();
win_sparkle_cleanup();

WinSparkle exits cleanly tearing down all its threads.

I should have confirmed my suspicion with a debugger first instead of opening an issue

I apologize for the noise.

Thanks again, Ryan

App calls win_sparkle_set_can_shutdown_callback to invoke QCoreApplication::quit ... My problem is caused by my code and a faulty can_shutdown_callback implementation that despite it's name apparently results in app.exec() exiting early ahead of RequestShutdown.

I don't know what to make of these remarks, both "despite the name" and the reference to calling quit. It looks an awful lot like you're misinterpreting the purpose of the "can shutdown?" callback, possibly because only seeing the name and not the docs:

/**
    Set callback for querying the application if it can be closed.
    This callback will be called to ask the host if it's ready to shut down,
    before attempting to launch the installer. The callback returns TRUE if
    the host application can be safely shut down or FALSE if not (e.g. because
    the user has unsaved documents).
    @note There's no guarantee about the thread from which the callback is called,
          except that it certainly *won't* be called from the app's main thread.
          Make sure the callback is thread-safe.
    @since 0.4
    @see win_sparkle_set_shutdown_request_callback()
*/
WIN_SPARKLE_API void __cdecl win_sparkle_set_can_shutdown_callback(win_sparkle_can_shutdown_callback_t callback);

I don't see any reasonable implementation of that that would exit an event loop. Typically, it would either just return true or check if any work is unsaved.

In any case, nothing in WinSparkle can exit your Qt event loop early, primarily because it knows nothing about it and (by design) runs on an entirely separate thread.

I don't know what to make of these remarks, both "despite the name" and the reference to calling quit.

Shoot! Great catch. Copy/paste mistake on my part.

That should have read: "App calls ~win_sparkle_set_can_shutdown_callback~ win_sparkle_set_shutdown_request_callback to invoke QCoreApplication::quit"

I've updated that step in the original report for clarification.

Also, wow. Thank you for taking the time to read and seriously consider in an already closed issue. 👍

In hopes to further clarify, I was actually doing two things:

Setting up win_sparkle_set_can_shutdown_callback to check if my App can can safely shutdown
Setting up win_sparkle_set_shutdown_request_callback to invoke QCoreApplication::quit

Originally, I didn't think the win_sparkle_set_can_shutdown_callback was relevant because it shouldn't be shutting down my app. So I never included or referenced its usage in the sequence of events I originally reported.

But it turns out that assumption was wrong. And we had actually written a very very buggy win_sparkle_can_shutdown_callback_t implementation that despite our best intentions caused app.exec() to exit ... 😬

So our buggy can_shutdown_callback was the real root cause. All the rest was just noise.

The WinSparkle docs are clear. And we understood the different / intent behind both callbacks. And there wasn't actually anything wrong with the shutdown_request_callback invoking QCoreApplication::quit.

WinSparkle is working as intended. This was purely user error.

So, again, thank you!

Both for your efforts developing and maintaining this library. But also for your attention to detail and taking the time to read (and even catch mistakes) in an already closed/resolved bug report.

And if nothing else, I've learned that I should really avoid submitting bug reports when a problem isn't fully understood in order to help avoid wasting everyone's time :)

Take care, Ryan

vslavik / winsparkle

Question: WinSparkle deadlock if automatic updates enabled and update manually triggered and installed #237