functions bound to self-updating Control properties get called even after Display is removed

[Ronny Lorenz]

It seems that once an inline function of a desklet is bound to a self-updating property of any Control it gets called continuously until eternity every time the control notifies about a change. Although this actually is the bahavior one wants and expects the function call continues even if the desklet is removed and thus the function is non-existent anymore. This in turn results in flooding the log with messages like these:

Warning: <function do_time at 0x3181230>(((10, 32, 57),)) has raised an exception
while running in main thread.
The error was:
'TargetCanvas' object has no attribute '_TargetCanvas__widget'
Traceback (most recent call last):
  File "/home/ronny/.local/lib/gdesklets/utils/__init__.py", line 181, in
run_in_main_thread
    return function(*args)
  File "<inline '/home/ronny/.gdesklets/Displays/clock-
desklet/clock.display_-4083978482491522790'>", line 70, in do_time
  File "/home/ronny/.local/lib/gdesklets/scripting/ElementWrapper.py", line 18, in
__setattr__
    self.__target(open).set_prop(name, value)
  File "/home/ronny/.local/lib/gdesklets/utils/Element.py", line 103, in set_prop
    setter(key, value)
  File "/home/ronny/.local/lib/gdesklets/display/TargetCanvas.py", line 232, in
_setp_graphics
    self.__render(value)
  File "/home/ronny/.local/lib/gdesklets/display/TargetCanvas.py", line 193, in
__render
    self.__redraw()
  File "/home/ronny/.local/lib/gdesklets/display/TargetCanvas.py", line 204, in
__redraw
    w, h = self.__widget.size_request()
AttributeError: 'TargetCanvas' object has no attribute '_TargetCanvas__widget'

Shouldn't the binding of a specific inline-script variable to an update callback be broken on desklet removal?

The bug can be reproduced with at least version 0.36.3 up to the latest bazar revision (187) by adding e.g. Martin Grimme's clock-desklet and then removing it again. (other deklets that use the time property of ITime control produce the same results for me)

[Joe Sapp (sappj)]

Sounds like all callbacks in a desklet aren't being removed when it is removed. This should definitely be fixed.

[Joe Sapp (sappj)]

Can you test to see if this patch addresses the issue? I'm debating between this way of doing this and another way - keeping track of all the bound functions within a desklet and writing an "unbind" function for Controls. This seems easier.

[Ronny Lorenz (raumzeit)]

Thanks for the patch!
Unfortunately no change after I applied it...

I am still trying to understand the internals of gDesklets. Once I find
out how the Control system works I will also look into fixing this bug.
Since I studied the preferences part of the gdesklets internals I still
dont know how the rest works. But as soon as I think that I figured it out
I look forward contributing.

Btw. an unbind function that may be called on display-remove sounds
quite good and clean.

Ronny

P.S.: I am totally new to this launchpad system of software development.
I filed a blueprint but dont know how to proceed and make the changes
available for the other developers. e.g. you, such that you may comment
on the new features or fixes.
Is there a guideline somewhere?

On 02/27/2012 08:37 PM, Joe Sapp wrote:
> Can you test to see if this patch addresses the issue? I'm debating
> between this way of doing this and another way - keeping track of all
> the bound functions within a desklet and writing an "unbind" function
> for Controls. This seems easier.
>
> ** Patch added: "bug_941337.patch"
> https://bugs.launchpad.net/gdesklets/+bug/941337/+attachment/2788736/+files/bug_941337.patch
>

[Joe Sapp (sappj)]

That's weird, I could've sworn that would work...
And the problem with my other method is that the desklet script directly accesses the control's bind() function. I don't know that there's a way to associate that call with a particular desklet so when it's being removed it can notify the control.

[Joe Sapp (sappj)]

Now I see the problem I had. The callback code still seems to be available in the sandbox. The function is called and it tries to update its display, but the display doesn't exist.

[Ronny Lorenz (raumzeit)]

Exactly as you described in #5!

I digged into it too, yesterday. But I dont see the the point where removal of the display does the wrong thing. I tried executing self.__script_stop() of the display/Display object before destroying the rest in the remove_display() method to ensure that all the sandbox stuff is shut down first. But this did not work. Although, stopping the script should actually involve stopping and destroying of all bound controls as well.

I also checked the call chain when a display is removed. The sandbox gets actually stopped, the trusted environment removed and also the control unload functions kick in. However, the gobject.timeout_add() function might be the problem, since it keeps calling the timer function of the desklet although it shouldn't.
What I found was that although the control should be removed, the timeout callback bound to gobject.timeout_add() does not return False. But it should, since _add_timer() of main/Control always checks if the self.__is_stopped property of the control object is set True.
Maybe the garbage collector isn't able to kick in properly since an update function is still bound to the gobject.timeout_add callback? Very weird, I will look into this one this evening again.

[Ronny Lorenz (raumzeit)]

Arrgh, I couldn't resist to check this control.stop() method again. There I found that actually TWO instances of the Control class might exist although a display just requested ONE. In the case of the ITime control, I see that the timeout of the control gets twice bound to gobject.timeout_add. Then, one of the controls seems to shutdown properly while the other still tries to do its job, i.e. updating the display widget that doesnt exist anymore.
I still dont know exactly what happens when a control is requested (scripting/Script, method __script_get_control() ) by the sandbox. Maybe the ControlWrapper does something strange while not creating a single instance but two at the same time?

[Ronny Lorenz (raumzeit)]

I think I found the source of the problem!
When a display is created and it makes use of a control, e.g. ITime, the __script_get_control() method of scripting.Script is called to return an instance of the appropriate control. When this happens, ControlFactory.get_control() already calls the constructor for the control object. At this point the timeout function of ITime kicks in and is passed to gobject.timeout_add(). Then the control is appended to the __loaded_controls attribute of Script().
So far so good.
But now, the control gets deep-copied by the ControlWrapper and a second instance of the control is created that also passes its timeout function into gobject.timeout_add().
When the display is then removed the controls created by the wrapper persist. When I circumvent the ControlWrapper and return the control obtained by ControlFactory instead everything works as expected. The control gets shut down and all its timers removed.
Is there any information about the purpose and use of the ControlWrapper somewhere in the repo?

[Joe Sapp (sappj)]

Good hunting!

The purpose of ControlWrapper is to allow multiple instances of Controls to exist in what appears to be a list to the desklet. It handles expansion and contraction. I just implemented it last year, so it figures it introduced a bug :) Perhaps ControlWrapper isn't deleting everything it needs to delete?

[Ronny Lorenz (raumzeit)]

The patch I just posted fixes the bug of 'lost' control wrappers.
Since it seems that we always create wrapped controls and never use the actual control instance (used as template for the wrapper) returned from ControlFactory this control 'template' is now stopped and deleted after it passed through the wrapper. Then, instead of appending the control 'template' to the list of loaded controls I introduced a new attribute __control_wrappers to the Script class where the wrappers get appended to.
On Script.stop() all wrappers are parsed and each wrapped control is shutdown properly and removed.

This bugfix may also influence memory leakage behavior of gDesklets. As far as I understand all the wrapped controls remained after reload or removal of a display even though new instances were created each time a display was initialized, piling up a heap of unusable unreachable control instances. The bugfix should remove that behavior in all circumstances.

Can anyone confirm my assumptions?

[Ronny Lorenz (raumzeit)]

Unfortunately it does not seem to fix bug #198401

[Ronny Lorenz (raumzeit)]

Just another thought about the bugfix...
I think a more clean way of getting rid of the wrapped controls is to introduce a __del__ destructor in the ControlWrapper class. This destructor can go through all stored controls and purge them. Then scripting.Script is able to just go through its list of stored wrappers and delete them one by one on call of the stop() method.

[Christian Meyer (chrisime)]

AFAIK the destructor __del__ isn't necessarily called in python.

2012/3/1 Ronny Lorenz <email address hidden>:
> Just another thought about the bugfix...
> I think a more clean way of getting rid of the wrapped controls is to introduce a __del__ destructor in the ControlWrapper class. This destructor can go through all stored controls and purge them. Then scripting.Script is able to just go through its list of stored wrappers and delete them one by one on call of the stop() method.

[Ronny Lorenz (raumzeit)]

Thanks for the hint, I just found out about this too.

In my first attempt to implement a 'clean' way of control wrapper removal I just called the __del__() method explicitely. However, this looks a bit strange. Maybe a purge_controls() method in the ControlWrapper class that has to be called right before deletion but also is ensured to be called by the destructor would be a good idea?
Anyhow, a user might then call the purge_controls() method but not delete the wrapper. If the wrapper is then used elsewhere it leads to a lot of exceptions if any of the __getattr__/__setattr__ methods are called.

I will think about it in more detail and maybe create a blueprint for a possible change in the ControlWrapper class

[Ronny Lorenz (raumzeit)]

I updated the patch since the first one I posted destroyed runtime extension (array increase) of the control wrapper. Now, on Script.stop() the wrapped controls get purged as well as the original copy of the control template

[Joe Sapp (sappj)]

Looks like you have a good start. I agree that storing the ControlWrappers (rather than the controls obtained by factory.get_control()) is the better thing to do.

Further, I think ControlWrapper should implement stop(), which would essentially do what you've outlined in your patch here. That way, we can keep scripting/Script.py mostly the same and ControlWrapper can stop all the Controls its allocated and the original one.

What do you think? Can you take a shot at this Ronny?

[Ronny Lorenz (raumzeit)]

Yes I agree.
I also think that the wrapper should know best what its going to do on stop(). I will implement this feature. However, once the stop() method of the wrapper is called should all Control instances be removed then or just be deactivated, i.e. stopped? Because we somehow should indicate that the wrapper has no active control instances stored anymore, which I would like to do by setting the __length attribute to 0 or even -1. But this would conflict with the current implementation where __length=0 makes the wrapper to behave like an ordinary control while __length > 0 exposes a list of controls.
I thought that actually we dont need to keep an original copy of a control. Wrapper length extension may also result in copying the last (or any other) already stored control since after deepcopy the new control is initialized to its defaults anyway. We then ensure that there is always at least one control instance in the list after setting the __length attribute to any positive integer value >= 0. (I am open for debate on this one, since storing a copy of the original control and using this as a template has the same effect, however, the original control should be stopped since it serves no function but beeing a template for active controls)

Calling the stop() method of the wrapper could then stop all controls, remove them from the internal control list of the wrapper and thus make the wrapper unusable until eternity.
On the other hand we could be so kind that on stop() all but the very last control are stopped and removed. Then the last control is just stopped and the __length attribute of the wrapper is set to -1 indicating no active controls. All wrapper methods should check for this new __length setting and respond appropriately. If a user then re-sets the length attribute because he/she decided to not give up on using the wrapped controls, re-initialization can take place to make the wrapper usable again. Since the stop() method may be callable by inline scripts of the user I prefer this way. On stop() of the entire Script instance all stored wrappers will be stopped and deleted in the hope that at some time later the garbage collector will get rid of them and thus the last remaining inactive controls.

I suggest that I make an implementation of this latter feature. Anyone disagreeing?

[Ronny Lorenz (raumzeit)]

Here is the patch that resolves this bug in the way I outlined before. It has taken a while since I was very busy with other stuff but it seems to work smoothly and in my opinion it also integrates much better into the gdesklets framework. The behavior is as follows:
A ControlWrapper can still behave like a list or like a single object depending on its length property. This is the behavior that was already present. What I changed now is that one can set the length to 0 via sandbox-script even when it was initialized with a value > 0 in the xml dom tree. The same ist true the other way arround. This also means that setting the length=1 still results in a list context for the ControlWrapper object while setting length=0 doesn't.
I also fixed a bug that appeared when the wrapper length was decreased. Anyway, when a wrappers length is decreased down to 0 the whole object does not behave like a list anymore and a warning message is thrown out to the log.
Another new behavior is that the original copy of the control is deactivated, i.e. stopped, after init of the wrapper. This ensures to have no (probably) silent timeout functions anymore.
On Script.stop() all ControlWrappers of the script get stopped via ControlWrapper.stop() which itself stops each wrapped control and deletes it.

I did not know if I should just commit the changes to the bzr branch on launchpad and also suspect that due to my changes some po/*.po files have to be altered because of line number changes. Is this correct?