search

hotwired / stimulus

A modest JavaScript framework for the HTML you already have
https://stimulus.hotwired.dev/
MIT License
12.74k stars 427 forks source link

Potential Memory Leak in Dispatcher #547

Closed redrockcity closed 2 years ago

redrockcity commented 2 years ago

I'm attempting to use Stimulus to implement infinite scroll for a list of results. As the page is scrolled, new pages of results are fetched and their elements added to the DOM. Some of these elements have stimulus controllers, actions, or are targets. As these elements are added to the DOM, Stimulus is correctly attaching controllers and event handlers to them as expected.

As an optimization, as the page is scrolled further, results that have moved off screen are removed from the DOM. Again, Stimulus is correctly detaching controllers and tearing down event handlers as elements are removed.

If the page is scrolled back up to view results that were previously removed from the DOM, they are refetched and reinserted into the DOM. If the page is scrolled up and down repeatedly, result elements will be removed, refetched, and reinserted as needed. The effect is that only the results the user is currently viewing (with a small buffer) are present in the the DOM at any time.

While running some tests of scrolling the page up and down, I noticed the heap size of the page continually growing. I did a heap snapshot and noticed a large amount of detached HTMLElements (representing the removed results) were still on the heap. Although they were not in the DOM, they were still referenced by the eventListenerMaps in the Stimulus dispatcher.

image

Over time, eventListenerMaps is continuing to grow with detached eventTargets even after they have been disconnected from a controller and removed from the DOM. My infinite scroll page certainly exacerbates this effect, however I would think it could still be an issue with long running SPAs or Turbo apps using Stimulus.

I've tried removing the result elements using parentElement.innerHtml = "", element.remove(), parentElement.removeChildren() but am seeing the same effect with eventListenerMaps. Is there a different way of removing elements such that Stimulus will not maintain a reference to them and they can be garbage collected?

Thanks!

tleish commented 2 years ago

Did you happen to read issue https://github.com/hotwired/stimulus/issues/489?

redrockcity commented 2 years ago

Did you happen to read issue #489?

I did read that issue and attempted to implement the same fix mentioned at the bottom. I looped through all the elements and removed any data-* attributes prior to removing them from the DOM—thinking Stimulus might clean up the event listener mappings to said elements. Unless I did something wrong, it didn't seem to have any effect. I still got an accumulation of detached element references in the eventListenerMaps over prolonged use of the page.

intrip commented 2 years ago

@redrockcity https://github.com/hotwired/stimulus/pull/592 might have fixed this, can you still reproduce using the latest StimulusJS version?

marcoroth commented 2 years ago

Going to close this as part of https://github.com/hotwired/stimulus/pull/592. @redrockcity please feel free to re-open this one if you are still experiencing this on the latest version of Stimulus. Thank you!