Timeout hotfix and improvements#12
Merged
Merged
Conversation
d0e0de6 to
34d551a
Compare
The previous method seems to work correctly but on higher timeout values causes wrong behaviors For example on linux arm devices a timeout of 20 seconds will be lowered to about 7 seconds
34d551a to
cd2c2fa
Compare
Contributor
Author
|
hi @ruuk it is possible merge it? in case you have abandoned the project please advise that we need to find another way to update this component |
ruuk
reviewed
Jul 18, 2020
cd2c2fa to
15968a4
Compare
ruuk
approved these changes
Jul 20, 2020
Owner
ruuk
left a comment
There was a problem hiding this comment.
Code looks great. When I get a chance I'll give it a quick test and merge after that.
Owner
|
Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR include a hotfix and three improvements
Hotfix
After a long time i found the problem that plagued Netflix add-on library sync and normal use on slower devices or long operations.
The problem is caused from a wrong behaviour of waitForReturn method,
the calculation of time elapsed seems to be correct but it is not done correctly,
is particularly noticeable with high timeout values on linux with ARM devices.
I ran tests on a linux ARM device with timeout set to 20 seconds, these are the results:
Original AddonSignals:
2020-07-09 14:02:31.640 T:3597636480 DEBUG: [plugin.video.netflix (1)] Execution time info for this run: make_call 7684 msFixed AddonSignals:
2020-07-09 14:09:06.093 T:3625513856 DEBUG: [plugin.video.netflix (1)] Execution time info for this run: make_call 20002 msThis resolve my opened issue: #11
Other tests has been made by other users from NF issue: CastagnaIT/plugin.video.netflix#716
Improvement 1
In the waitForReturn method, was used:
xbmc.sleep(100)to delay the check,the problem is that in the cycle it almost always happens that the data arrives at the time of sleep
so the delay will be reflected in the add-on that receives the data
so if an add-on makes 5 calls results of:
5 * 100ms = 500ms of total delay
10 * 100ms = 1 seconds of total delay
it might be okay if an add-on only makes a few calls (like 1, 2) but this slows down the whole add-on system
then now has been lowered to
xbmc.sleep(10)Improvement 2
There is no system to really manage AddonSignals timeout in add-ons
so i have introduced the possibility of raising a timeout exception
as it should normally happen
This allows not only to better manage the timeout on add-ons side,
but allows you to return 'None' as value in the callback,
and thus avoid annoying workaroud for return empty values
Improvement 3
There is a borderline case that when using high timeout values,
the original loop do not take in account when kodi is closed,
therefore Kodi terminate the other add-ons but remains in a catatonic state
because the script remains in the loops and preventing Kodi from stopping completely
i see now that Matrix branch is another,
so this PR need also a foreward to Matrix