Widget destruct / cleanup

[vsivsi]

Hi, I'm new to TW5 and I've just run into the same problem as this chap @felixhayashi did: https://groups.google.com/forum/#!searchin/tiddlywikidev/widget$20cleanup/tiddlywikidev/yuQB1KwlKx8/GqAQVdiu470J

TLDR; If you create a new type of widget that requires any kind of cleanup when the parent Tiddle is closed, there appears to be no way to accomplish this short of hooking a DOM MutationObserver onto the ancestor div.tc-tiddler-frame.

Besides being generally yukky, that mechanism is brittle/incomplete because it may turn out that the parent Tiddle is itself embedded within another Tiddle, and so on... So unless the Widget constructor walks up to the document root to discover all of the containing tiddler frames and sets up an observer on each one, it may still fail to clean itself up. (Also the DOM may change after the widget is loaded, making even that approach too simplistic)

This may all seem pretty academic, but there are a lot of cool and powerful types of widgets that effectively are out of reach without a foolproof destructor mechanism:

1. A d3.js stock market chart that updates with live data using a websocket to some other server
2. A photo widget that periodically updates by requesting a new picture from an external source
3. A weather widget that reports the current conditions by polling a REST API
4. An SVG based multi-timezone clock that changes based on the ticking of setInterval() callbacks.

All of these (and thousands of other possibilities) have two things in common:

1. they update periodically with dynamic content (cool!)
2. they need a reliable mechanism to clean-up when the tiddle they are in is closed (boring but essential).

As I said, I'm new to this, having just successfully written my first dynamic updating widget. It works great, but leaks resources like a sieve. Anyway, if I've completely missed a straightforward way this can be done within the current framework, please educate me; otherwise I think it's time to start talking about how to properly implement destructors for widgets.

[felixhayashi]

Just saw this and wanted to link to the google discussion I once started then realized I am this "chap" you are talking about :D

It works great, but leaks resources like a sieve.

Due to the lack of a destructor, I was forced to add a periodic (~500ms) routine check for the TiddlyMap plugin whether a widget is still contained in the document: https://github.com/felixhayashi/TW5-TiddlyMap/blob/master/src/plugins/felixhayashi/tiddlymap/startup.caretaker.js#L233

I highly agree that destructors are needed because external libraries sometimes require the user to destruct objects and how should we do it when we do not know when our widgets are removed... Anyway, the routine check I mentioned above is a workaround...

-Felix

[vsivsi]

@felixhayashi Thanks for the up-vote and the link to your workaround. I hope we can all agree that if people are resorting to polling the DOM in a system with perfectly serviceable events/messages, then some key functionality is missing!

[vsivsi]

As a quick follow-up... I've been messing around with removeChildDomNodes() in the widget base class. It seems to work to simply move the highlighted lines below out of the else clause so that this call propagates to all child widgets, giving them the opportunity to override the base widget class implementation and get notified: https://github.com/Jermolene/TiddlyWiki5/blob/fdbde1b389a46df684cd89bb876df843860d150e/core/modules/widgets/widget.js#L473-L476

A quick search yields no core widgets that override this method, and preliminary testing shows that everything seems to work great with this change. If desired, I'll put together a PR with the 2 line change for others to try out, or I can just use the shadow for my own project going forward. Anyway, thought I'd share my solution, seems much preferable to polling or adding MutationObservers all over the place!

[vsivsi]

Here's my actual code to implement the above idea in widget.js:

/*
Remove any DOM nodes created by this widget or its children
*/
Widget.prototype.removeChildDomNodes = function() {

    // Call the destructor on this widget
    this.destructor();

    // If this widget has directly created DOM nodes, that were not handled by 
    // the destructor (still in this.domNodes) delete them now.
    if(this.domNodes.length > 0) {
        $tw.utils.each(this.domNodes,function(domNode) {
            domNode.parentNode.removeChild(domNode);
        });
        this.domNodes = [];
    }

    // Ask the child widgets to delete their DOM nodes
    $tw.utils.each(this.children,function(childWidget) {
        childWidget.removeChildDomNodes();
    });
};

/*
Stub destructor
*/
Widget.prototype.destructor = function() {
};

Then in your own widget, you can simply override the base widget class stub destructor:

/*
Implement a local destructor
*/
MyWidget.prototype.destructor = function() {
    console.log("Destroy everything!");
};

What do you think?

[pmario]

@Jermolene

[felixhayashi]

Hi @vsivsi

/* 
Remove any DOM nodes created by this widget or its children
*/
Widget.prototype.removeChildDomNodes = function() {

    // Call the destructor on this widget
    this.destructor();
    ...

I think this is not the right place for a destructor call here. You want the child widgets to be able to respond to the destruction - not the widget itself.

So a function

MyWidget.prototype.destruct = function() {
    console.log("Destroy everything!");
}

is fine and every widgets would need this function but the actual destructor call needs to be done when iterating over the child widgets for every child widget... Have a look at Widget.prototype.renderChildren(). It iterates over the child widgets and asks them to render. A destruct is the exact antogist.

So I think we need something like this:

/*
Rebuild a previously rendered widget
*/
Widget.prototype.refreshSelf = function() {
    var nextSibling = this.findNextSiblingDomNode();
    this.destructChildWidgets();
    this.removeChildDomNodes();
    this.render(this.parentDomNode,nextSibling);
};

And

/*
Destruct the children of this widget
*/
Widget.prototype.destructChildWidgets = function() {
    $tw.utils.each(this.children,function(childWidget) {
        childWidget.destruct();
    });
};

-Felix

[sukima]

I think this is a higher priority need. I too felt adding a polling was bad form. :disappointed:

[vsivsi]

@felixhayashi

Good catch, I agree that the order is wrong, it should probably be:

/*
Remove any DOM nodes created by this widget or its children
*/
Widget.prototype.removeChildDomNodes = function() {

    // Ask the child widgets to destruct and delete their DOM nodes
    $tw.utils.each(this.children,function(childWidget) {
        childWidget.removeChildDomNodes();
    });

    // Call the destructor on this widget
    this.destructor();

    // If this widget has directly created DOM nodes, that were not handled by 
    // the destructor (still in this.domNodes) delete them now.
    if(this.domNodes.length > 0) {
        $tw.utils.each(this.domNodes,function(domNode) {
            domNode.parentNode.removeChild(domNode);
        });
        this.domNodes = [];
    }
};

I don't think it needs to be any more complicated than this. Each widget can implement a destructor function or not, if it's omitted then the do nothing stub from the base widget class will be used.

For widgets that have state needing clean-up, they can call their own destructors before re-rendering when data they depend on has changed. No need for a separate mechanism to handle that. As you pointed out, the rerendering order is already determined by the existing base widget class implementation.

[felixhayashi]

Hi @vsivsi

I agree that the order is wrong, it should probably be

I think you are right with this rearrangment. Now the the widget that is at the end of the widget tree can destruct before its parent...

I don't think it needs to be any more complicated than this.

I agree, just thought it would make sense to separate dom removal and destructor stuff but it is very much the same so your suggested change seems reasonable to me and I am in favour of it - unless Jeremy spots a mistake ;)

[Jermolene]

Hi @vsivsi and welcome to the project.

I do favour adding support for widget destructors although I note that we will need some careful testing to be confident of these changes.

However, it's worth noting that I wouldn't recommend all of your original examples be written as a widget. Even with destructors implemented, widgets are intended to be lightweight components; we create and tear them down all the time, often with each keypress in an editor. The problem with a widget making HTTP requests is the likelihood that the widget will have been refreshed before the response comes back. The alternative way of implementing these things is a startup module or daemon.

[buggyj]

I think the way to go is 're-use' the message mechanism and allow 'downwards messages' to be propagated down the widget tree. We could have a tm-final or similar message that widgets could listen for.

Then when widgets that re-create themselves or remove child widgets in some other way would call dispatchDownEvent({type: "tm-final"}) before removing child widgets.

[felixhayashi]

Hi @buggyj

I think messaging is not suited here because it's asynchronous and a parent widget should wait/pause till the destruct function of a child was executed.

[felixhayashi]

Or does TiddlyWiki directly execute dispatched events right when dispatch() is called? (@jermolene)

edit Having looked at https://github.com/Jermolene/TiddlyWiki5/blob/fdbde1b389a46df684cd89bb876df843860d150e/core/modules/widgets/widget.js#L372 it seems that tw-message-events are not pushed to a stack and executed asynchronously but instead executed right away! So @buggyj's idea to introduce a downward bubbling is theoretically possible.

[sukima]

Confused. All widgets have a synchronous render method. It is part of the widget interface. The system calls that function when needed.

Why not the same thing. When the system is removing a widget from the DOM it first calls the tearDown or destroy method if that widget defines one otherwise business as normal.

The idea of "re-use the event mechanism" which is designed for an asynchronous event bus seems like overkill for a simple interface change. In fact the code has a removeChildDomNodes already. Just add a Widget.prototype.remove function and make sure the core system calls it. Then if you want to use it override it in your own widget code or not.

Am I missing something?

[felixhayashi]

@sukima

I agree with you, I also think that the event mechanism is not the correct way. I just said its theoretically possible to use it...

I am with @vsivsi idea which is exactly what you described above.

[vsivsi]

I've been using the code in this comment for a couple of weeks and it's working great: https://github.com/Jermolene/TiddlyWiki5/issues/1784#issuecomment-111254331

[buggyj]

The event mechanism is synchronous. But maybe using it for destroy is not needed.

After re-reading the thread it seems that removeChildDomNodes() Is the destroy method, and that all that a widget would need to do is override this method if other resources need to be removed, or states need to be saved. However, as suggested in this thread, the default method of removeChildDomNodes would need to call childWidget.removeChildDomNodes(); on all it child widgets before removing its own dom nodes.

The only advantage to using a tm-final message is that the current implementation of removeChildDomNodes() would not have to be changed, but it would be more complex to implement messaging.

[tobibeer]

Is this ready for a pull request?

[buggyj]

Firstly, I also have re-order the calls of removeChildDomNodes() in my tiddlywikis and found no problems. However this is not the same as showing that for all updates of the widget tree that this function is called (some analysis of the widgets needs to be done to show that widgets that reoganise the widget tree alway call removeChildDomNodes() before removing widgets.)

Logically I would expect a destructor to remove domnodes, so maybe we should rename removeChildDomNodes() to distroy() and have that function call distroyChildren()?

[sukima]

How about

/**
 * Optional method that is called just before DOM nodes are to be removed.
 * This give an opportunity to cleanup resources such as removing DOM event
 * handlers which would otherwise linger causing a memory leak.
 */
Widget.prototype.cleanup = function() {
};

See also: http://javascript.info/tutorial/memory-leaks

[felixhayashi]

Is this ready for a pull request?

@ all, I created a solution which tries to incorporate all previous thoughts in the discussion. Please see #2280