Fix deadlock when using xbmcgui.DialogProgress in plugin #1097

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Member

pieh commented Jun 21, 2012

This fixes http://trac.xbmc.org/ticket/13113

Currently xbmc deadlocks when using xbmcgui.DialogProgress in plugin before we setup DialogProgress in https://github.com/xbmc/xbmc/blob/master/xbmc/filesystem/PluginDirectory.cpp#L481 :

  • CPluginDirectory::WaitOnScriptResult is blocking GUI thread while waiting for plugin results and it will only run render loop (progressBar->Progress(); on L518) if we wait 1500 ms and don't have active DialogProgress
  • script can't finish because closing ProcessDialog (from python: either directly via xbmcgui.DialogProgress.close() or on python's obj dealloc) needs running render loop

This commit simply adds processing render loop while waiting for plugin results even if we don't show DialogProgress from CPluginDirectory::WaitOnScriptResult.

while waiting for plugin results - process render loop always, not ju…
…st when showing own's plugin progress dialog


fixes #13113
Owner

MartijnKaijser commented Jun 21, 2012

Tested and confirmed fixed.

Member

jmarshallnz commented Jun 21, 2012

This has the potential for nastiness, so we need to be careful we get things right.

If you're calling a directory, then you're already potentially off-thread here already I think? Perhaps with the current GetDirectory background stuff, we no longer need the progress dialog here at all? Reducing the potential for clashes seems reasonable. @elupus your thoughts?

It should be OK if you're calling from setResolvedURL() which I think is the other spot this kicks in?

Member

elupus commented Jun 21, 2012

My first thought was just that this stuff is not run in main thread anyway.
So should not need to run rendering loop. Ive not looked at the patch yet
though. Will try to Sunday.

Member

pieh commented Jun 21, 2012

well, then current solution should be wrong too becouse line i'm replacing is doing exactly the same (what I'm adding is just doing it also when we don't have progressBar set) - see https://github.com/xbmc/xbmc/blob/master/xbmc/dialogs/GUIDialogProgress.cpp#L97

--edit:
just as bonus: here's backtrace of deadlock: http://pastebin.com/t889JHQN (in this case it seems I'm editing code that runs through GUI thread - which might not always be the case as You pointed)

Contributor

nuka1195 commented Jun 22, 2012

Would you consider adding the ability for a plugin to override this progress dialog and create their own as part of this fix.

Reason being is I want to give more feedback to the user for an initial setup which can take a considerable amount of time.

Or at least the ability to set the percentage and lines for the current dialog.

Member

pieh commented Jun 22, 2012

@nuka1195
This involves adding python bindings which is not something I know much about ... and this wouldn't really fix this problem.

I believe main problem here is that we should run renderloop just as main application loop. So IMO proper fix would be getting completely rid of CGUIWindowManager::ProcessRenderLoop and instead of current approach - queue background jobs (for time-consuming tasks) / specify callbacks (for dialogs DoModal()) to not block GUI thread (easy to say, heh)

Member

pieh commented Jul 24, 2012

Did some more checks and it seems that g_windowManager.ProcessRenderLoop(false); will only do anything if g_application.IsCurrentThread() is true so it seems safe to use in non-gui threads.

Of course this doesn't look nice so I was just thinking if we could add some kind of callback to IDirectory and then concrete implementations could annouce progress via this callback:
in CPluginDirectory::GetDirectory (or any other implementation) we could do something like:

m_callback->setProgressInfo(true); // this would cause using progress dialog instead of busy dialog in off-thread getdirectory
m_callback->setHeading(my_plugin_name); 
m_callback->updateProgress(progress, some_string_info_about_progress);

if (m_callback->abortRequested()) // if user cancels we could smartly stop getting directory (if possible that is)

also, maybe this could allow plugins to provide their own progress info (with more work on python side I guess)

Member

jmarshallnz commented Oct 13, 2012

This should be taken care of in master as we no longer throw up the progress.

Member

pieh commented Oct 13, 2012

Actually that doesn't fix this issue - we are blocking gui thread while in loop in CPluginDirectory::WaitOnScriptResult() and script can't finish because it need to progress dialog to close (which can't be done because we aren't processing render loop). I will reopen PR when I'll have nicer fix.

Member

jmarshallnz commented Oct 13, 2012

Does it occur only when we're calling the script to setResolvedUrl() ?

Member

pieh commented Oct 14, 2012

I don't know what setResolvedURL does so can't answer that.

This happens when we run CPluginDirectory::WaitOnScriptResult() in app thread (so when we want to run plugin items marked as playable). Look here: https://github.com/XBMC-Addons/plugin.video.the.trailers/blob/master/addon.py#L243 there is called xbmcgui.DialogProgress, but problem is we aren't really running renderloop now, and because of that pDialog.close() will never return (we need renderloop running for that). xbmc.sleep(500) workarounds this by waiting for progress dialog from CPluginDirectory::WaitOnScriptResult to show up (and we start running renderloop there: progressBar->Progress(); ) and then addon "takes over" that dialog and overwrirte dialog labels and progress bar with own data.

IMO proper way would be:

  1. Get totally rid of progress bar in CPluginDirectory::WaitOnScriptResult and move calling addon to background thread (similliar like off thread GetDirectory is done).
  2. Don't use xbmcgui module in xbmc plugins and provide methods to provide progress feedback, get input from user via xbmcplugin module. This wouldn't be needed after 1. to fix this issue, but if/when we will support serving plugins via UPnP this will be needed, so remote client wouldn't cause showing dialogs on xbmc instance acting as upnp server (this can propably be easy produced now with calling GetDirectory / PlayMedia to plugin path that will use xbmcgui from json rpc).
Member

jmarshallnz commented Oct 14, 2012

setResolvedURL is a callback for plugin:// URL items that are marked playable, so probably this case.

Right - it's not necessarily the progress bar, rather that we're on app thread but not spinning the process loop. Ofcourse, we have to be here, so that's not really an issue. Given that, I suggest we have a check for app thread, and if so, make sure the app loop is run (i.e. exactly like CDirectory::GetDirectory does).

Whether the progress or busy is used isn't really a big deal IMO - I'd prefer the latter as it's more consistent with other things (it's not cancelable I think?) and is less likely to cause issues with the plugin also throwing up other dialogs (agreed they shouldn't be doing this, but we can't stop them at this point for Frodo).

So, I think just cleaning this up a bit so it's the same as CDirectory::GetDirectory would be the best way forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment