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

Clear dangling EventListeners and Detached Nodes when a controller is removed from the DOM #592

Merged
merged 1 commit into from Oct 31, 2022

Conversation

intrip
Copy link
Contributor

@intrip intrip commented Oct 20, 2022

eventListenerMaps maps HTML elements handled by Stimulus to eventListenerMap, a map of Stimulus EventListeners in this way: { HTMLelement: { eventName: EventListener } }. When a controller HTML is removed from the DOM the dangling EventListeners aren't removed, moreover, the removed HTML elements are still referenced via the eventListenerMaps keys. This leads to having multiple detached HTMLelements and eventListeners which can't be GCed hence a memory leak. The leak is fixed by removing the dangling eventListeners and clearing unused eventListenerMaps keys. When the HTMLelement is attached again to the DOM, the eventListenerMaps and the related eventListeners are automatically re-created by Stimulus via the MutationObservers, no data is lost by doing this.

Below is an example to reproduce the issue:

<html>
  <head>
    <title>EventListenerMaps memory leak</title>
    <script type="module">
      import { Application, Controller } from "https://unpkg.com/@hotwired/stimulus/dist/stimulus.js"
      window.Stimulus = Application.start()
      Stimulus.register("boosts", class extends Controller {
        doClick() { process(500) }
      })
    </script>
    <script>
      function process(count) {
        let i = 0
        let handler = setInterval(function(){
          if (++i > count) {
            clearInterval(handler)
          } else {
            document.body.replaceWith(document.body.cloneNode(true))
          }
        }, 1)
      }
    </script>
</head>
<body>
  <div data-controller="boosts">
    To reproduce:
    <ul>
      <li>Check heap snapshot</li>
      <li>Click "trigger leak" button</li>
      <li>Check heap snapshot again</li>
    </ul>
    <button data-action="click->boosts#doClick">trigger leak</button>
  </div>
</body>
</html>

heap snapshot before:

Screenshot 2022-10-19 at 17 10 35

heap snapshot after:

Screenshot 2022-10-19 at 17 11 10

Related #489 #547

@rik
Copy link
Contributor

rik commented Oct 20, 2022

Could this be solved by only changing Dispatcher.eventListenerMaps to a WeakMap?

this.eventListenerMaps = new Map()

@intrip
Copy link
Contributor Author

intrip commented Oct 20, 2022

Could this be solved by only changing Dispatcher.eventListenerMaps to a WeakMap?

this.eventListenerMaps = new Map()

I thought about this too but, unfortunately, I don't think it would work: EventListener objects (referenced via EventListenerMaps have a specific lifecycle so they need to be correctly disconnected before being removed, by using a WeakMap we don't have control on that. For example, If an EventListener won't be disconnected properly we'll have a case when the Dispatcher will re-create and connect a new one and we'll end up in a state with duplicate EventListener objects connected to the same event/target.

Copy link
Member

@jorgemanrubia jorgemanrubia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work tracking this one down @intrip 🙌!

@@ -7,7 +7,7 @@ import { Token, ValueListObserver, ValueListObserverDelegate } from "../mutation

export interface BindingObserverDelegate extends ErrorHandler {
bindingConnected(binding: Binding): void
bindingDisconnected(binding: Binding): void
bindingDisconnected(binding: Binding, clearEventListeners?: boolean): void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a scenario where we want to invoke this bindingDisconnected method without clearing the listeners?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! I added this conditional after I've seen this test to fail: When an attribute is replaced with a new one bindingDisconnected is triggered (via tokensUnmatched) but if we clear the listener, in this case, we lose the options initially set ("once" in this case) so the test will fail.


if (eventListenerMap.size == 0) this.eventListenerMaps.delete(eventTarget)
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe could extract a couple of methods to clarify logic. I also added a method EventListener.hasBindings:

private clearEventListenersForBinding(binding: Binding) {
  const eventListener = this.fetchEventListenerForBinding(binding)
  if (!eventListener.hasBindings()) {
    this.removeMappedEventListenerFor(binding)
    eventListener.disconnect()
  }
}

private removeMappedEventListenerFor(binding: Binding) {
  const { eventTarget, eventName, eventOptions } = binding
  const eventListenerMap = this.fetchEventListenerMapForEventTarget(eventTarget)
  const cacheKey = this.cacheKey(eventName, eventOptions)

  eventListenerMap.delete(cacheKey)
  if (eventListenerMap.size == 0) this.eventListenerMaps.delete(eventTarget)
}

@@ -51,6 +52,19 @@ export class Dispatcher implements BindingObserverDelegate {
this.application.handleError(error, `Error ${message}`, detail)
}

private clearEventListenersForBinding(binding: Binding) {
const eventListener = this.fetchEventListenerForBinding(binding)
if (eventListener.bindings.length == 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this check eventListener.bindings.length == 0? In which scenario can this be invoked where this doesn't validate? I don't see anything wrong I just want to understand 😅.

Copy link
Contributor Author

@intrip intrip Oct 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please ask!
It's a rough edge but It's possible to have multiple bindings for the same EventListener. Let's say we have this scenario:

<div data-controller="boosts">
  <button data-action="click->boosts#doSomething click->boosts#doSomething">Hey!</button>
</div>

In this case, if change the "data-action" we don't want to clear the EventListener unless we remove both click->boosts#doSomething actions.
Right now we call this method only if the whole controller is removed so the safeguard is not a necessity, it was a necessity in the first version I made which didn't use the "clearEventListeners" flag. On the other hand, keeping it looks a bit safer to me so right now I'm keen on keeping it. No strong opinion at all though (can even add a force flag if needed at some point to skip the guard).

@brunoprietog
Copy link

Great, this would be able to potentially fix #489 and #547?

@intrip
Copy link
Contributor Author

intrip commented Oct 21, 2022

Great, this would be able to potentially fix #489 and #547?

Yes, those two issues look related.

… removed from the DOM

`eventListenerMaps` maps HTML elements handled by Stimulus to `eventListenerMap`, a map of Stimulus
EventListeners in this way: `{ HTMLelement: { eventName: EventListener } }`.
When a controller HTML is removed from the DOM the dangling EventListeners aren't removed,
moreover, the removed HTML elements are still referenced via the `eventListenerMaps` keys.
This leads to having multiple detached HTMLelements and eventListeners which can't be GCed
hence a memory leak. The leak is fixed by removing the dangling eventListeners and clearing
unused `eventListenerMaps` keys. When the HTMLelement is attached again to the DOM,
the `eventListenerMaps` and the related `eventListeners` are automatically re-created
by Stimulus via the MutationObservers, no data is lost by doing this.

Below is an example to reproduce the issue:

```
<html>
  <head>
    <title>EventListenerMaps memory leak</title>
    <script type="module">
      import { Application, Controller } from "https://unpkg.com/@hotwired/stimulus/dist/stimulus.js"
      window.Stimulus = Application.start()
      Stimulus.register("boosts", class extends Controller {
        doClick() { process(500) }
      })
    </script>
    <script>
      function process(count) {
        let i = 0
        let handler = setInterval(function(){
          if (++i > count) {
            clearInterval(handler)
          } else {
            document.body.replaceWith(document.body.cloneNode(true))
          }
        }, 1)
      }
    </script>
</head>
<body>
  <div data-controller="boosts">
    To reproduce:
    <ul>
      <li>Check heap snapshot</li>
      <li>Click "trigger leak" button</li>
      <li>Check heap snapshot again</li>
    </ul>
    <button data-action="click->boosts#doClick">trigger leak</button>
  </div>
</body>
</html>
```
@intrip intrip force-pushed the clear-dangling-event-listeners branch from 4a064a2 to a912acc Compare October 31, 2022 11:40
@dhh dhh merged commit 8b8b79b into hotwired:main Oct 31, 2022
This was referenced Nov 1, 2022
@intrip intrip deleted the clear-dangling-event-listeners branch November 2, 2022 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants