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

Owned weak event listener to prevent memory leak (using EventListenerOptions) #243

Closed
duanyao opened this issue May 9, 2016 · 12 comments
Closed
Labels
needs implementer interest Moving the issue forward requires implementers to express interest

Comments

@duanyao
Copy link

duanyao commented May 9, 2016

The problem

In order to implement a UI component such as popup menu or tooltip, it is usually necessary to add an event listener on an ancestor node (e.g. document) to hide/remove the component when the user click/touch outside of the component. However, it is a significant burden to make sure the event listener on the ancestor node is removed when the component is to be hidden/removed. For example, if some uncareful code removed one of the ancestor nodes of the component from the tree without also remove the listener on document, then the listener is leaked.

It would be nice if the event listener on the ancestor node is automatically removed by UA after the component is removed.

Prior art: weak event listener

There is no concept of weak event listener in DOM, HTML or ECMAScript standards yet, though ActionScript 3 and .Net already implemented it to prevent leak of listener. In short, a weak event listener is an event listener object that is weakly referenced by the event source and/or the runtime. If the weak event listener is no longer strongly referenced by user's code except the the event source, it is legitimate to be removed from the event source and GC'ed.

Weak event listener can be useful to solve the problem mentioned above: we can add an event listener weakly on the ancestor node and let the component reference the event listener strongly. After the component is removed and GC'ed, the event listener will also be removed from the ancestor node.

However, weak event listener exposes indeterministic behavior of GC to user's code: weak event listener is still working after it is no longer strongly referenced by user's code but not GC'ed yet.

Proposed solution: owned weak event listener

This solution is described as follow:

  • An event listener added to a DOM Node has an optional owner, which is also a DOM Node. (Currently I think non-DOM events are not difficult to manage so are out of scope).
  • If an event listener has an owner, and the owner is not in the active document, it will not receive any event; if the owner is re-inserted into the the active document, the listener may receive events again.
  • If the event listener and its owner are both no longer strongly referenced by user's code, the UA may remove the listener from its event target and GC the listener and its owner.

To implement this feature, the owner Node should hold strong references to its owned event listeners, and the event target Node may hold weak references to the turple of event listener and its owner.

Note that this feature is deterministic: an event listener won't be GC'ed before its owner can be GC'd; The the event listener is turned off when its owner is removed from the document, not when it is GC'ed.

The API looks like:

dictionary AddEventListenerOptions: EventListenerOptions {
  //...
  Node owner = null;
};

To solve the problem mentioned at the beginning, just let the owner be the component:

function showPopupComponent(popup, x, y) { // popup: Element
  popup.style.position = 'absolute';
  popup.style.left = x + 'px';
  popup.style.top = y + 'px';
  document.body.appendChild(popup);
  // no need to care about removing this event listener,
  // it will stop to work after the popup is removed and will eventually be GC'ed
  document.addEventListener('click', ev => {
      if (!popup.contains(ev.target)) {
        document.body.removeChild(popup);
      }
    },
    { owner: popup }
  );
}
@markcellus
Copy link

markcellus commented May 9, 2016

I get the intent and the goal here, but I think the approach adds a level of assumption that may not be worth implementing, especially for, imo, is such a small use case. This also makes it easy to lead to bad practices, which is already easy to do in the world of JavaScript, unfortunately. :)

I guess my point here is that removing listeners when they are added should be required always if the desire is to prevent memory leaks. It sounds like your intent here is to have to write less code, which is arguable of whether thats a good enough reason. Regardless of the reason though, I think the code should at the very minimum require an engineer to explicitly opt-in for this functionality, rather than assume that an engineer wants it, similarly to WeakMap or WeakSets.

Additionally, passing an owner doesn't make it clear of what its actually being used for or why such a property needs to exist, which is essentially to keep an internal mapping to remove the listener later. But fixing that may just be changing the property name or similar... or, again, finding a more efficient way to explicitly opt-in for this functionality like mentioned above.

@duanyao
Copy link
Author

duanyao commented May 9, 2016

I guess my point here is that removing listeners when they are added should be required always if the desire is to prevent memory leaks.

Actually, if the event target and the event listeners have same life time (or event listeners live longer), this is usually not required -- GC of event target usually means GC of event listeners. This is the case if all event listeners of a UI component are only added to component the and its children. But it can be problematic if the event target outlives its event listeners, e.g. event target is document, as described in my first post.

JS is a language with GC, but manual event listener management sometimes defeats the goal of GC. This proposal trys to make event listeners more GC-friendly.

Additionally, passing an owner doesn't make it clear of what its actually being used for or why such a property needs to exist

I choose "owner“ to mean "Although this event listener logically belongs to Node x, it has to be added to one of x's ancestors. I want this event listener to die after Node x dies."

I think your code should at the very minimum require an engineer to explicitly opt-in for this functionality

This functionality does require opt-in: if you omit the owner property, it is a traditional event listener.

@markcellus
Copy link

markcellus commented May 9, 2016

Sorry for the confusion. I'm speaking of only scenarios where the event target outlives its event listeners. You see this a lot in single page web applications which are beginning to be more common, so event listening GC does not apply for these scenarios.

Two things come to mind with the owner:

  1. Based on the proposal, the owner will always be fixed and, once passed, cannot change. What happens if DOM is manipulated (parent is changed) between the time you pass it to add listener and the time you remove it?
  2. Is there any value of passing an owner? when that owner can be implied already by querying parentNode or similar?

@duanyao
Copy link
Author

duanyao commented May 9, 2016

I think developers should avoid event target outliving its event listeners where possible, no matter SPA or not. If not possible, try owned weak event listener :). If all failed, I think it means the app is not modulized well.

I think owner can be changed by call addEventListener() again with a different owner. Per spec, only capture of EventListenerOptions affects the identity of event listener.

when that owner can be implied already by querying parentNode or similar?

How? The event target and event listener don't known the logical owner of the event listener unless we specify one.

@annevk annevk added the needs implementer interest Moving the issue forward requires implementers to express interest label Mar 16, 2017
@victornpb
Copy link

I like the idea of having the possibility to have weak listeners, it can be useful in some cases, but It can trick you into think it would prevent a leak, but in fact it would cause one.

Lets say you added a event on document to close a popup, and the you remove the popup from the DOM, the GB trashes the listener, this means the listener never gets called, but you used that listener to teardown your thing and maybe sub components, but now they will live forever as ghosts "components".

@duanyao
Copy link
Author

duanyao commented Aug 10, 2017

@victornpb Sorry for later reply. You are right. Never assume a weak event listener will be triggered at least once.

@annevk
Copy link
Member

annevk commented Mar 13, 2018

I think you need to have more compelling evidence that this is a desirable feature. E.g., show adoption of it by widely used frameworks and popular sites.

@duanyao
Copy link
Author

duanyao commented Mar 14, 2018

@annevk
I am not aware of such frameworks or sites. This feature is probably not polyfillable until ES weak referecne is avaible.

One of the evidence outside web platform is weak event in .net:
https://docs.microsoft.com/en-us/dotnet/framework/wpf/advanced/weak-event-patterns
https://www.codeproject.com/Articles/738109/The-NET-weak-event-pattern-in-Csharp
https://stackoverflow.com/questions/13788787/when-to-use-weak-events

@annevk
Copy link
Member

annevk commented Mar 14, 2018

You can certainly implement something that deactivates event listeners when a corresponding node is removed. You might even simply remove the listener. You'd lose ordering, but you'd at least not leak. In any event, I'm going to close this. More experience by libraries is needed for this to be considered.

@annevk annevk closed this as completed Mar 14, 2018
@duanyao
Copy link
Author

duanyao commented Mar 15, 2018

@annevk I'm not sure what you mean. If I want to implement

document.addEventListener('click', ev => { ... }, { owner: popupElem }); 

I have to reference popupElem strongly somewhere, and this effectively prevent popupElem from being GC'd when it is no longer needed elsewhere.

@annevk
Copy link
Member

annevk commented Mar 15, 2018

If popupElem controls adding and removing the listener you don't have that problem.

@duanyao
Copy link
Author

duanyao commented Mar 15, 2018

It is not always possible. E.g. one of the ancestor of popupElem is removed before popupElem could clean up the listener.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs implementer interest Moving the issue forward requires implementers to express interest
Development

No branches or pull requests

4 participants