Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PluginDirectory: dont time out while script is modal/not cancelled #10093

Merged
merged 1 commit into from
Jul 12, 2016

Conversation

tamland
Copy link
Member

@tamland tamland commented Jul 8, 2016

Complete fix for #10080. Nothing in foreground (not just modals) should time out and attempted stopped.

Also, since scripts can end at any time, it should not block the caller for such long time waiting on completion (as it did not do before, so this brings back the old behaviour).

@tamland tamland added the Type: Fix non-breaking change which fixes an issue label Jul 8, 2016
@FernetMenta
Copy link
Contributor

FernetMenta commented Jul 8, 2016

I don't see that this does fix anything. I strategy was that the scripts gets killed if it does not return within a given timeout.

@@ -486,35 +485,26 @@ bool CPluginDirectory::WaitOnScriptResult(const std::string &scriptPath, int scr
CScriptObserver scriptObs(scriptId, m_fetchComplete);
if (!CGUIDialogBusy::WaitOnEvent(m_fetchComplete, 200))
{
cancelled = true;
m_cancelled = true;

This comment was marked as spam.

@Tolriq
Copy link
Contributor

Tolriq commented Jul 9, 2016

@FernetMenta I did not test your last iteration but if

Also, since scripts can end at any time, it should not block the caller for such long time waiting on completion (as it did not do before, so this brings back the old behaviour).

Is true it should for sure be fixed, a 1 sec script should not take 30sec to return, specially since in my case I timeout at 30 so if there's too much data or slow network to return the JSON data it will fail 100% of the time :( Would be a big regression.

@FernetMenta
Copy link
Contributor

@Tolriq

Also, since scripts can end at any time, it should not block the caller for such long time

where do you see this in the current code? the code checks if the script is still running in a loop. As soon as the script terminated the caller is not blocked anymore.

@Tolriq
Copy link
Contributor

Tolriq commented Jul 9, 2016

@FernetMenta small typo it was if true, I was quoting @tamland so I supposed he have tested and report that because it was true. As I said not at home so was not able to test your iteration :)

@tamland
Copy link
Member Author

tamland commented Jul 9, 2016

Having a timeout like this is not a good idea because:

  • There's a million different things plugins can do that are perfectly "fine" and should not time out, not just modal and dialogs. Including that it can just be naturally long running.

  • When it's in the foreground user is already giving consent to continue and can cancel it at any moment. Timeout in this case have no use.

  • Lastly, there seem to be some confusion around what 'stop' does. It does in no way reliably "kill" the script. There is only one scenario where it will work: when the script isn't doing any blocking operation.

    In other cases it's only going to cause deadlocks and corruption. Therefore it should be avoided until you absolutely have to. That's why the 30 wait is there. Since directory fetch happens in the background there's no need to rush and stop it.

@tamland
Copy link
Member Author

tamland commented Jul 9, 2016

Also, since scripts can end at any time, it should not block the caller for such long time waiting on completion (as it did not do before, so this brings back the old behaviour).

Is true it should for sure be fixed, a 1 sec script should not take 30sec to return, specially since in my case I timeout at 30 so if there's too much data or slow network to return the JSON data it will fail 100% of the time :( Would be a big regression.

I was referring to the 1 second wait for completion in the loop, nothing as dramatic as that;). It if ends (as opposed to complete) it's a 1 second gui delay we don't need.

@FernetMenta
Copy link
Contributor

When it's in the foreground user is already giving consent to continue and can cancel it at any moment. Timeout in this case have no use.

If this is always the case, I agree with you. But I don't see that this is the case. If the script brings up an additional window, the user can't cancel the busy dialog.

@tamland
Copy link
Member Author

tamland commented Jul 9, 2016

If this is always the case, I agree with you. But I don't see that this is the case. If the script brings up an additional window, the user can't cancel the busy dialog.

Example on how to produce that? I just tried with a script opening a dialog from a plugin and it seem to work just fine. The busy dialog can be cancelled both before and after.

@FernetMenta
Copy link
Contributor

I was thinking that plugins intercept user actions but this does not seem to be the case. Worst that can happen is that a user has to push back twice, first to close the dialog opened by the plugin, then to cancel the busy dialog. Is this correct? If so, you are right and an additional timeout makes indeed not much sense.

@tamland
Copy link
Member Author

tamland commented Jul 12, 2016

As far as I can tell, that’s the case. So should we go with this?

@FernetMenta
Copy link
Contributor

yes, please go ahead with this.

@tamland
Copy link
Member Author

tamland commented Jul 12, 2016

jenkins build this please

@tamland tamland merged commit 5568fd3 into xbmc:master Jul 12, 2016
@tamland tamland deleted the fix_plugin_dir branch July 12, 2016 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Fix non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants