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
[python] add xbmc.abortEvent #5217
Conversation
@jimfcarroll for code review and idea for documentation. Maybe there's a better way of doing this so also JSON-RPC can benefit from it like the other using the monitor class. (https://github.com/xbmc/xbmc/blob/master/xbmc/interfaces/legacy/Monitor.h) @Montellese ? I'm sure that JSON-RPC users would welcome knowing if device will be shutdown. Edit: like the other python addition, at the same time the python API should be bumped. |
Good to see this, thanks. I started a forum thread a while back which included some ideas, that may or may not be useful. I'm guessing takoi == @tamland? |
Is there a reason you couldn't just add this to the ModuleXbmc.h/cpp as a static methods instead of applying it through python? The only reason I didn't do it that way myself is because all of the scripts that are using the variable, but if we're changing the API anyway, why not just make the change in the module? |
@MilhouseVH Yep @jimfcarroll I initially tried to reuse xbmc.sleep but gave up on that because you need communicate directly with module from |
I mean you would write it in terms of a CEvent and expose it in terms of several static methods on ModuleXbmc. If you can write it in a few simple C++ calls on ModuleXbmc it will "just work" in python. That way, if I ever get around to my grandiose plans of adding another scripting language it will "just work" there also. |
Yeah, all those other languages.. Whatever happened to that? You mean expose isSet/wait/set and keep the PythonInvoker stuff? That could work. You'll lose the nice name space though.. Did generating attributes for variables with swig ever get solved? |
@MartijnKaijser: not sure what you are referring to concerning JSON-RPC? |
like that, yes. |
@Montellese i mean that remotes for example could listen through JSON-RPC that it's going to shutdown. I see that's already added as "System.OnQuit" ? http://wiki.xbmc.org/index.php?title=JSON-RPC_API/v6#System_3 Edit: perhaps strive to having similar names for function between JSON and python if we ever would generate them from a a single source. (it was something mentioned iirc) |
@tamland: Yeah I've added that functionality a while ago so you should be able to use it. If we want to generate JSON-RPC and python APIs from the same source we'll have to re-write the whole API. IMO in this case the new property should be named as close to the existing one as possible so e.g. |
@jimfcarroll I don't think this will work. When you call the c++ library the GIL will be held until the method returns. You can't interrupt it via python. I'm guess this is why the sleep method is implemented the way it is. |
The old python code is considered legacy. So IMO if we combine json RPC and
|
You have the option of whether or not to hold the GIL by using the DelayedCallGuard (it releases the GIL). If you can write it in C++ with static methods in ModuleXbmc then it will work in python. Alternatively you can create a class called something like an AbortRequest and retrive a singleton from a static call on ModuleXbmc. As far as those "other languages" ... I got busy with a new job. I'm sure I'll get to it after the zombie apocalypse takes care of other day-to-day issues :-) ... Every once in a while I try to figure out how to merge JSON-RPC with the python stuff and I run away with my tail between my legs. :-) |
Perfect. :-) I just sat down to do this myself (of course, I was going to create an AbortRequest class but whatever). This looks good to me. It's more verstile than setting it internal to the python vm. Thanks. I'll let someone else merge it. 👍 |
*/ | ||
void stopSFX(); | ||
|
||
/** |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
if @jimfcarroll is ok this can go in the next merge window i guess. @tamland please include the needed python API bump. |
@@ -418,7 +418,19 @@ namespace XBMCAddon | |||
* example: | |||
* - language = xbmc.convertLanguage(English, xbmc.ISO_639_2) | |||
*/ | |||
String convertLanguage(const char* language, int format); | |||
String convertLanguage(const char* language, int format); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@tamland sorry I just noticed I didn't scroll down enough. Unless I'm mistaken, the PythonInvoker::stop call doesn't need to include the CallMethod to python. You should be able to ditch the rename in the .i file and just directly call ModuleXbmc::setAbortRequested in the PythonInvoker. Am I missing somethng? |
@jimfcarroll Yeah.. where the h is this that supposed to be declared? All this magic is making me dizzy.. Anyway, updated!:) |
jenkins build this please |
still missing the python API bump |
No it's not. It's in the first commit. |
oops. sorry that i missed that. thx. |
Perhaps I've misunderstood the purpose of this PR. With the following code as an example of a typical "service.py" addon:
Then, when using this new They are being called when the "old" Is this behaviour intentional? If so, this would mean the new Other than that, it works nicely! |
Yeah, this isn't going to work. There's an even bigger issue: I though this was loaded for each script but it's not. It's global. Stopping one service will stop all and state is kept across executions. The original PR worked as intended (not receiving callbacks though). |
@MilhouseVH yes. I thought about this when @tamland first proposed a new abort mechanism. Currently there are only 2 methods that ever allowed for callbacks. 'sleep' and 'doModal'. At one time I had modified this so that ANY call to a long running xbmc method had the potential to execute a callback but unfortunately several poorly written tempermental scripts broke so I put it back to only those 2. I personally I think what your suggesting makes perfect sense and it should do that. I saw this as just a new mechanism for watching for the abort as opposed to the current boolean that's totally python specific and difficult to do in the codegenerator. Changing this would be easy. If @tamland wants to do it then he needs to make the waitForAbort look like the ModuleXbmc::sleep method. Look at the sleep method. To make the callbacks execute you need to call MakePendingCalls on the current "LanguageHook" but you cannot be in the scope of the "DelayedCallGuard" but you can get the current "LanguageHook" instance from it. Again, if you look at the sleep method that description should make sense. If you want me to do it I can wait until this goes in. It should take about 10 minutes. |
Okay. How is this called on a per-script basis from within the PythonInvoker::stop method? |
Right above where |
(By the way, if my testing is premature, don't hesitate to tell me to back off! ;-)) This is so nearly working perfectly, many thanks. When using an infinite timeout, the methods are firing as they should, and the waitForAbort() method is working well, both when the system is shutting down (ie. notifying all waiting services) and also when disabling an individual service. However, when a specific timeout value is used, disabled services begin to interact with enabled services (or maybe vice versa). I've created two very simple services, download link. The zip contains both All that these two services do is block on waitForAbort(), spinning round if a This is
You can configure the When both services are using a When However, once When Sample output is below, using a 2.0s
A possibly related issue is that if
|
@MilhouseVH I'm unable to reproduce this. Are you sure you aren't just seeing |
@MilhouseVH did you set the addon id differently in the 2 scripts? @tamland nice job. This looks like the right solution to me - once it's working. EDIT: @MilhouseVH - looks like you did in the scripts you zipped up. |
I could not reproduce this. It looks like it's working as intended. @MilhouseVH what platform are you on? |
@MilhouseVH when one script is running with an infinite timeout the other one is the only thing writing to the logs for long periods and since the log messages are not disambiguated the log messages will be aggregated into a "the last line repeats XXX times" message. I think this explains what LOOKS like "sporatic" behavior. |
Oh wow, wow... what a muppet! :( Yes, sorry about the rate limiting in the log - I had just been grepping it, which meant I missed the "last line repeated n times" message! D'oh. I've now retested after adding a unique count to the log message (test ZIP updated), and this is now working as expected, in every respect! Great job! Argh. /hides. |
At least it's extensively tested now:D |
IMO this is good to go whenever someone wants to take it. |
jenkins build this please |
[python] add xbmc.abortEvent
Not sure if I'm doing it wrong, but onAbortRequested() is not called when quitting XBMC. Diff[1]: - while not xbmc.abortRequested:
- xbmc.sleep(100)
+ if xbmc.__version__ >= '2.19.0':
+ log('waitForAbort')
+ service.monitor.waitForAbort()
+ else:
+ log('while...xbmc.sleep')
+ while not xbmc.abortRequested:
+ xbmc.sleep(100) The log file says it's using waitForAbort(). If I'm not missing something, I'd expect it to call onAbortRequested() when XBMC quits. |
@anaconda You won't receive callbacks after the main thread dies. It's no different from your old version. I don't think onAbortRequested have ever worked as intended. |
Unfortunately, this PR is the cause of a sigabrt when exiting certain screensavers (eg. Nayancat or Unary Clock). With this PR removed, specifically b2e6e50 reverted, there is no sigabrt. To test, install Nayancat or Unary Clock screensaver from XBMC.orgs repo. Go to Settings -> Appearance -> Screensaver, select the Nayancat/Unary Clock screensaver, then click on Preview. Wait a few seconds for the screensaver to start, then click a button/key to exit the screensaver - xbmc.bin will abort if it includes b2e6e50. Ubuntu Backtrace (tested by fritsch on #irc): http://paste.ubuntu.com/8342961
and gdb backtrace from OpenELEC/R-Pi:
|
Wow. This is really bad and will be hard to find. I'll see what I can do today. |
For me, Unary Clock doesn't crash at all, Nayancat does however. Looking the add-ons I have a hunch this has something to do with weird self deleting inside a callback they both do. Swig doing something weird when it's deleted early? |
Yeah.. just add a bunch of members to the Monitor class and it will crash without this patch. |
I started looking into this. There is a double free taking place in a non-AddonClass (probably something indirectly referenced from the Monitor class or a Monitor callback). It's nasty. Turning on reference tracking and trace logging causes the problem to disappear. I'll try to keep people posted. |
Still working on it. Adding leak and double-free detection the hard way. If anyone cares to see, I'm working here: https://github.com/jimfcarroll/xbmc/commits/scribbler-fix |
Okay. I'm pretty sure I got this: #5381 |
@tamland you were right. It was the deleting inside the callback. But it wasn't SWIG, it was me. That should fix it. |
I intend this as a simple replacement of the boolean property. It allows add-ons to usexbmc.abortEvent.isSet()
instead, and most importantlyxbmc.abortEvent.wait()
instead of periodically checking.Missing:
- Documentation: surely this is currently done by magic, so I'm hoping at least the abortEvent attribute should be documented.abortRequested
: need elaborate hacks in initialization script to do this.