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.

Sign up for GitHub

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

Jump to bottom

memory-leak with basic-app for done-ssr@2 #560

Closed
cleong-tc opened this issue Aug 2, 2018 · 9 comments · Fixed by #566
Closed

memory-leak with basic-app for done-ssr@2 #560

cleong-tc opened this issue Aug 2, 2018 · 9 comments · Fixed by #566
Assignees
@matthewp
Labels
bug

Comments

@cleong-tc
Copy link
Contributor

cleong-tc commented Aug 2, 2018 •
edited
Loading

i am getting a memory-leak for [email protected] (possible since v1.2.0-beta.0) for a basic-app...

src/index.stache...

<html>
  <head>
  </head>
  <body>
    <can-import from="my-app/app" export-as="viewModel" />
		index.stache

		<script src="/steal.production.js"></script>
  </body>
</html>

hitting with ab -c 5 -n 999 http://localhost:8080/, i see CanSimpleDocument not being cleared in DevTools memory-profiler via three-snapshot-method ( https://www.alexkras.com/simple-guide-to-finding-a-javascript-memory-leak-in-node-js/ ).

memory_leak-basic_app

there might be an additional memory-leak when using a dynamic-import.
src/index.stache...




<html>
  <head>
  </head>
  <body>
    <can-import from="my-app/app" export-as="viewModel" />
		index.stache

		<can-import from="my-app/mobile/index/">
			{{#isResolved}}
				<mobile-index />
			{{/isResolved}}
		</can-import>

		<script src="/steal.production.js"></script>
  </body>
</html>

hitting with ab -c 5 -n 99 http://localhost:8080/, i see Promise (and CanSimpleDocument) not being cleared.

memory_leak-dynamic_import

for done-ssr@1, the cause of the dynamic-import memory-leak (mentioned in canjs/can-zone#178 ) was resolved by reverting from...

var DOCUMENT = require("can-globals/document/document");

to...

var DOCUMENT = require("can-util/dom/document/document");

in node_modules/can-view-import/can-view-import.js for [email protected]+ (but does not resolve the memory-leak for done-ssr@2).
@matthewp matthewp added the bug label Aug 3, 2018
@matthewp
Copy link
Contributor

matthewp commented Aug 3, 2018

So maybe a leak in can-globals. I'll check this out now..

@matthewp
Copy link
Contributor

I'm looking into this again. Have been on something else the last week and a half.

@matthewp
Copy link
Contributor

Here's one cause: canjs/can-dom-data-state#21

This has ripple effects that causes an event listener to be registered with can-globals several times. This might be the source of the CanSimpleDocument leak, but I haven't confirmed that yet.

@matthewp matthewp self-assigned this Aug 15, 2018
@matthewp
Copy link
Contributor

Any cause of the leak is that can-view-live listens for when DocumentFragments are removed from the DOM, which of course never happens (their children can be removed). I'm looking into the best way to fix that.

@matthewp
Copy link
Contributor

An update: I was able to find most of the sources of the leak. The biggest problem is in done-ssr where our cleanup code runs outside of the Zone, which causes can-dom-mutate to get the wrong document, and therefore is unable to call all of the teardown functions. This is easily fixed.

I'm still seeing a small leak but I think I know what's causing it, will update here once everything is confirmed.

@matthewp matthewp mentioned this issue Aug 16, 2018
Merged
@matthewp
Copy link
Contributor

Pull request: #566

@matthewp
Copy link
Contributor

Reopening as I don't think that PR fixes all leaks. I'm still seeing some leakage from tests, although greatly reduced. I'm going to release a patch version of done-ssr with those leaks fixed and will continue searching.

@matthewp matthewp reopened this Aug 16, 2018
matthewp added a commit that referenced this issue Aug 22, 2018
@matthewp
Prevent leak caused by saving promise with can-dom-data-state
d25e9b9 
This prevents a leak caused by storing a promise with
can-dom-data-state. Since this instance of can-dom-data-state comes from
Node's module system and not Steal's, this doesn't get cleaned up when
we clean up memory with can-dom-mutate.

In this case we can replace the usage with Symbols.

This commit also turns back on the memory leak test, which is now
passing. Closes #560
@matthewp matthewp mentioned this issue Aug 22, 2018
Merged
@matthewp
Copy link
Contributor

Found the other leak. #569 fixes it. Re-enabled the memory leak test and there are no more leaks. Will release a patch once that passes.

@matthewp
Copy link
Contributor

2.1.5 is out and all leaks are fixed: https://github.com/donejs/done-ssr/releases/tag/v2.1.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@matthewp matthewp

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Clean up memory correctly donejs/done-ssr
2 participants
@matthewp @cleong-tc