Skip to content

Avoiding memory leaks#734

Closed
andrija-hers wants to merge 2 commits intowebsockets:masterfrom
allexjs:master
Closed

Avoiding memory leaks#734
andrija-hers wants to merge 2 commits intowebsockets:masterfrom
allexjs:master

Conversation

@andrija-hers
Copy link
Copy Markdown

Here is a massive refactor that minimizes memory leaks.

There are 2 important notes here:

  1. This PR will not work until Avoiding closure leaks einaros/options.js#14 is accepted (the options module leaks memory massively)
  2. One of the test still fails - just like the original code does (due to timeout of 5000 ms)

I hope that @einaros will take care of the options repo so that this PR may be accepted.

The code changes are pretty much self explanatory, but I'll just note that almost all the work is based on removing the notorious var self = this paradigm, that always leads to memory leaks (via closures that never get collected as garbage).

@3rd-Eden
Copy link
Copy Markdown
Member

Might make more sense to dump the options module completely.

@3rd-Eden
Copy link
Copy Markdown
Member

As for the var self = this -- We're changing to fat arrow syntax so that should be resolved in master.

@einaros
Copy link
Copy Markdown
Contributor

einaros commented Jun 29, 2016

Regarding the Options pullreq, I can accept it, but at first glance it seems unnecessary. When references to an Options instance are released, everything - including the closures - should be gc'd.

@JacksonTian
Copy link
Copy Markdown
Contributor

@andrija-hers could you rebase it with master

@andrija-hers
Copy link
Copy Markdown
Author

@JacksonTian I'll do my best, asap

@lpinca
Copy link
Copy Markdown
Member

lpinca commented Jan 11, 2017

Is this still needed? We no longer depend on options and are no longer using the var self = this; paradigm.

@lpinca
Copy link
Copy Markdown
Member

lpinca commented Jan 27, 2017

I'm going to close this as there would be a pretty big amount of work only to rebase against master.
Thanks a lot for the effort you put here @andrija-hers.

@lpinca lpinca closed this Jan 27, 2017
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.

5 participants