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

Only install change detectors if has document cookies #425

Closed
wants to merge 1 commit into from

Conversation

meros
Copy link

@meros meros commented Dec 20, 2023

Closes: #419

@MINWONAHN
Copy link

MINWONAHN commented Dec 22, 2023

Your code is Goooooood!!
How about changing the code as below to eliminate the fundamental cause of memory leaks?

AS-IS

  private _startPolling() {
    this.pollingInterval = setInterval(this.update, 300);
  }

  private _stopPolling() {
    if (this.pollingInterval) {
      clearInterval(this.pollingInterval);
    }
  }

TO-BE

  private _startPolling() {
    this._stopPolling(); // to remove pollingInterval that cause memory leak;
    this.pollingInterval = setInterval(this.update, 300);
  }

  private _stopPolling() {
    if (this.pollingInterval) {
      clearInterval(this.pollingInterval);
    }
  }

@meros
Copy link
Author

meros commented Dec 22, 2023

This is not the reason for the leak (references earlier comment). The leak springs from the interval keeping a reference to this, not letting that object expire. Combine this with other code that assumed that it is ok to not unregister Change listeners (including the express middleware) and you get leaks.

Now, this PR is not a perfect fix, but in any case we don't need the interval unless HAS_DOCUMENT_COOKIES is true. So this change is good in any case...

@MINWONAHN
Copy link

The leak springs from the interval keeping a reference to this, not letting that object expire.

I believe that _startPolling must be cleared before setInterval(this.update, 300); because if you repeatedly call _startPolling without clearing it, setInterval can cause a memory leak.

  private _startPolling() {
    this.pollingInterval = setInterval(this.update, 300);
  }

@meros
Copy link
Author

meros commented Dec 22, 2023

The leak springs from the interval keeping a reference to this, not letting that object expire.

I believe that _startPolling must be cleared before setInterval(this.update, 300); because if you repeatedly call _startPolling without clearing it, setInterval can cause a memory leak.

  private _startPolling() {
    this.pollingInterval = setInterval(this.update, 300);
  }

Yeah, well ok but that is not the reason for the leak. There is a guard when calling _startPolling/_stopPolling functions to avoid these being called multiple times. Again, the reason for the leak is that the interval that is running has a reference to this, which keeps the object alive even if no one else has a reference.

eXon added a commit that referenced this pull request Dec 30, 2023
* Clean up test setup

* Migrate deprecated jest function

* Add tests

* Improve test
@eXon
Copy link
Collaborator

eXon commented Dec 30, 2023

Merged in #431

@eXon eXon closed this Dec 30, 2023
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.

Memory leak with v6
3 participants