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

fix: Fixing issue with leaking event listeners #668

Merged
merged 6 commits into from
Aug 29, 2023
Merged

Conversation

patrickhousley
Copy link
Contributor

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.

@github-actions
Copy link

github-actions bot commented Aug 29, 2023

Asset Size Report

Merging this pull request will result in the following asset size changes:

Agent Asset Previous Size New Size Diff
lite loader 27.67 kB / 9.69 kB (gzip) 27.67 kB / 9.69 kB (gzip) 0% / 0% (gzip)
lite async-chunk 44.31 kB / 14.7 kB (gzip) 44.51 kB / 14.73 kB (gzip) 0.47% / 0.24% (gzip)
pro loader 44.85 kB / 15.15 kB (gzip) 44.85 kB / 15.15 kB (gzip) 0% / 0% (gzip)
pro async-chunk 63.76 kB / 20.51 kB (gzip) 63.97 kB / 20.55 kB (gzip) 0.33% / 0.18% (gzip)
spa loader 51.27 kB / 17.1 kB (gzip) 51.27 kB / 17.1 kB (gzip) 0% / 0% (gzip)
spa async-chunk 78.06 kB / 24.82 kB (gzip) 78.27 kB / 24.86 kB (gzip) 0.27% / 0.16% (gzip)
lite-polyfills loader 98.11 kB / 31.38 kB (gzip) 98.11 kB / 31.38 kB (gzip) 0% / 0% (gzip)
lite-polyfills async-chunk 54.04 kB / 16.34 kB (gzip) 54.25 kB / 16.39 kB (gzip) 0.39% / 0.28% (gzip)
pro-polyfills loader 117.7 kB / 37.25 kB (gzip) 117.7 kB / 37.25 kB (gzip) 0% / 0% (gzip)
pro-polyfills async-chunk 94.21 kB / 25.67 kB (gzip) 94.42 kB / 25.72 kB (gzip) 0.22% / 0.19% (gzip)
spa-polyfills loader 125.67 kB / 39.38 kB (gzip) 125.67 kB / 39.38 kB (gzip) 0% / 0% (gzip)
spa-polyfills async-chunk 109.2 kB / 30.24 kB (gzip) 109.41 kB / 30.28 kB (gzip) 0.19% / 0.14% (gzip)
worker loader 39.79 kB / 13.56 kB (gzip) 39.79 kB / 13.56 kB (gzip) 0% / 0% (gzip)
worker async-chunk 43.3 kB / 14.77 kB (gzip) 43.51 kB / 14.81 kB (gzip) 0.48% / 0.31% (gzip)

@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Merging #668 (27141b8) into main (ec113af) will increase coverage by 0.09%.
The diff coverage is 95.65%.

@@            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              
Flag Coverage Δ
integration-tests 86.68% <100.00%> (+0.15%) ⬆️
unit-tests 81.73% <94.11%> (+0.28%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/common/timer/interaction-timer.js 96.36% <93.33%> (+5.25%) ⬆️
src/common/session/session-entity.js 86.66% <100.00%> (-1.00%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@metal-messiah metal-messiah left a 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

src/common/session/session-entity.js Outdated Show resolved Hide resolved
Copy link
Contributor

@cwli24 cwli24 left a 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.

@patrickhousley
Copy link
Contributor Author

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.

image

@github-actions
Copy link

github-actions bot commented Aug 29, 2023

Static Badge

Last ran on August 29, 2023 16:17:27 CDT
Checking merge of (27141b8) into main (ec113af)

@patrickhousley
Copy link
Contributor Author

jil tests passed here

@patrickhousley patrickhousley merged commit 6cb8238 into main Aug 29, 2023
36 of 37 checks passed
@patrickhousley patrickhousley deleted the fix-mem-leak branch August 29, 2023 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InteractionTimer leaks event listeners / debounces
3 participants