Deadlock while closing the window with persistent threads running

[pomplesiegel]

When updating the DOM from a separate thread, using calls like webview.evaluate_js() I found it quite useful to have an API call which indicates whether the window is still present / attempting to close.

Problem

If one clicks the close button on a window (rather than quitting it like with Apple + Q), and then a separate thread calls to the webview object, like webview.evaluate_js(), this thread becomes blocked and stuck in that call indefinitely.

What would be helpful is a function like
webview.window_exists()

which returns a boolean, which is set within WindowDelegate::windowWillClose_() (and the equivalent location on other platforms). I tested this by adding a global variable to cocoa.py which is set within windowWillClose_, and having webview return the status of this boolean. Certainly not the cleanest way, but it works.

Could we perhaps integrate this in a more OO-friendly fashion. I'm happy to help.

Alternatively, we could provide an API for a callback function, which would be called (in addition to the window closing) when the user hits the "x" or close button on a window. A sort-of cleanup call, to prevent deadlock.

Thoughts?
Thanks!
Michael

[shivaprsd]

Hm, looks like this is an issue we should solve internally, rather than exposing
it to the API. pywebview already handles calls that occur before the window is
created; the same could be done for calls that occur after. Another way is to
really quit the app once the window is closed, just as if ⌘Q was pressed.

I think the cleanup-call feature is more plausible, especially in the later context.
This would be an addition to the already implemented confirm_quit parameter.
Thanks for the suggestion, I'll look into it and get back to you. 🙂

BTW to check if the app is still running on Cocoa, you can use the isRunning()
method of BrowserView.app, not need to keep a global variable.

[pomplesiegel]

That would be great. Any thoughts of the equivalent call for WinForms?

[shivaprsd]

Roman does all Windows coding so I don't know much about it :) Nevertheless,
there doesn't seem to be an easy built-in way for this on WinForms; a global var
should be the best option.

[r0x0r]

The deadlock must certainly be fixed, but I don't feel there is a need for a public window_exists function (unless multi-window functionality materializes). A better approach would be to catch windowWillClose/OnDestroy/etc event on each platform and a set a semaphore there, evaluate_js would acquire the semaphore and would check for the existence of the window before executing javascript code.

[shivaprsd]

I just noticed, in its current shape, the multi-window code automatically handles
this. The get_instance method there raises an exception if the user calls an API
on a non-existent window, which breaks the deadlock. Here's a screenshot:

Setting a new semaphore would need adding it to all the API methods, right?

[pomplesiegel]

@shivaprsdv, Any thoughts on the best path forward for this one? I have a single-window application, but should I use the multi-window branch simply for this behavior? Currently there is no elegant way to quit a program with persistent threads running.

[shivaprsd]

I have confirmed that we have indeed hit a bug here. This occurs on all platforms
I have tested. A fix for Cocoa is up, could get some help with testing. It internally
uses the same isRunning() function that I mentioned.

@pomplesiegel Feel free to experiment with the multi-window branch anyway.
Even if your app is single-window, it should not incur any overheads. I wrote that
code, with inputs from Roman and others; it is still in testing, so any feedback is
well appreciated. 🙂

[pomplesiegel]

Great, i'll give the multi-window a shot. Thanks!

[pomplesiegel]

@shivaprsdv, I just tested your branch "deadlock-fix" and it fixed my thread problem for quitting the program! Great work.

[shivaprsd]

Great, thanks. 🙂

But I should say it's likely to be only a temporary fix. For the problem seems to
be of a much general nature, and warrants a more solid, cross-platform solution.
We'll keep this issue open and updated.

[r0x0r]

I have pushed a similar solution for WinForms to the deadlock-fix branch. Good to merge?

[shivaprsd]

Don't think so; we haven't had a solution to the problem on Linux yet.

Moreover, I don't think this approach would fix it for sure. I'm studying it in
detail, will report as soon as I can.

[shivaprsd]

Okay, I have pin-pointed the issue. It is two fold:

1. We are allowing API functions to be called even after closing the webview.
2. If the webview is suddenly closed, with a semaphore acquired from one of
   the subthreads, it might not always get released.

Ideally, such calls should be blocked in the init file itself. This benefits all the
platforms, and would be compatible with the work on multi-window as well.

Keeping this in mind, I have redone the work on this issue: you can find it in
the deadlock-fix-alt branch. I have added an internal is_running() method to
check if the webview has been closed. Please read the commit messages.

I've also added a tentative commit for Winforms based on your last commit,
feel free to amend it.

[pomplesiegel]

@shivaprsdv, great. so far this is working well on Mac. Any reason not to expose is_running() to the public API? I see it as being helpful as a check for a thread before it attempts to call a webview function.

[shivaprsd]

Nothing particular. As I mention in the above commit message, is_running
could be removed in the future. But if you do want, simply follow the Python
EAFP principle:

try:
    webview.evaluate_js()
    # webview is running
except:
    # webview is not running

[pomplesiegel]

Great, that's what I'm doing now. Thanks!

[shivaprsd]

@pomplesiegel Can you test the deadlock-fix and deadlock-fix-alt branches on
Windows and tell us if any of them solve the issue? Thank you.

[pomplesiegel]

Hello @shivaprsdv, sorry for the delay! I have tested both branches and am currently running the deadlock-fix-alt branch in my program. It's behaving very well! I have tested on both mac and windows. I will continue testing.

[r0x0r]

@shivaprsdv Looks good. Would you make a PR?