-
Notifications
You must be signed in to change notification settings - Fork 44
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
fix: Fixing issue with leaking event listeners #668
Conversation
Asset Size Report
Merging this pull request will result in the following asset size changes:
|
Codecov Report
@@ Coverage Diff @@
## main #668 +/- ##
==========================================
+ Coverage 86.78% 86.87% +0.09%
==========================================
Files 131 131
Lines 5032 5053 +21
Branches 634 635 +1
==========================================
+ Hits 4367 4390 +23
+ Misses 620 618 -2
Partials 45 45
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good minus the console log, nice find
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a question, but did you find that our wrap-events does not check that it's already wrapped? I ask because I thought wrap-function checks the flag
and doesn't re-wrap.
Good catch @cwli24 I did miss the underlying issue. So the actual problem is in the InteractionTimer where we add a listener to the events EE. When the session times out, we create a new InteractionTimer but the old one never gets fully destroyed because this listener is never destroyed. In fact, not only is this a memory leak, but it also hangs onto the session entity reference and, when an event happens like a click, the session entity is refreshed multiple times, once for every listener created due to the session expiring. |
jil tests passed here |
Fixing an issue where session timeout would result in rewrapping global event handlers. For instances where the site would be open for long periods of time, such as kiosks, this would eventually result in a degradation in event handler performance.
Overview
When our session entity timed out, it would re-initialize and re-wrap event handlers. For pages that stay open for long periods of time, this would eventually result in a noticeable performance degradation for event handlers on the page.
Related Issue(s)
https://issues.newrelic.com/browse/NR-137712
Closes #556
Testing
To reproduce this issue, or test the fix, the easiest way is to change the default session timeout values to 30s for the inactivity timer and 60s for the session timer. If you are reproducing the issue using what is currently in main, add console logs to each of the session entity methods to show when those methods are called. What you will find is that those method calls increase with each session timeout. With the fix in place, the number of method calls for a session reset stays static.