bcoe / sandcastle

A simple and powerful sandbox for running untrusted JavaScript.
MIT License
222 stars 48 forks source link

Memory skyrocketing in the sandbox child processes #39

Open seubert opened 10 years ago

seubert commented 10 years ago

In simple terms I have a Node process that spins up some SandCastles (either one or a few via the Pool functionality) and waits for some scripts as input to run. However, even if I never call Script.run() the sandbox processes themselves (not my process specifically, the children) jump up in memory usage over time. I tried to delve into all the vm source, but it's a little over my head and a very deep dive so I figured I'd ask here as well. Is there some GC that isn't happening that should? Any workaround recommendations, outside of 'spin up the child processes on demand'?

buildmaster commented 10 years ago

this is possibly/probably the heartbeat script running in the background. I'll take a look as soon as I get some downtime.

seubert commented 10 years ago

I assumed so as well, but I'm not entirely sure of a good way to fix it. I'll keep looking, but I'd appreciate any help too. Thanks :)

joernroeder commented 10 years ago

I ran into the same issue a couple of weeks ago and tried to fix it in my own branch https://github.com/joernroeder/sandcastle/tree/memoryLeakFixing. We did a tiny hackaton and stopped the experiment with the current state where the memory doesn't grow anymore (by forcing the gc to jump in). It's defenitly not a clean solution but we hadn't more time and I would help to improve it …

bcoe commented 10 years ago

@joernroeder would it be possible to get a pull request in for this fix, even if it's a bit rough I'd love to eliminate a memory leak :+1: also, you have another pull request outstanding, is that good to merge (if so go ahead and do so).

seubert commented 10 years ago

@joernroeder @bcoe if the tasks don't get put into master soon, I hoisted the memory leak fixes into what's currently on master and saw my problems vanish. The expose_gc stuff is a bit iffy but it works and I haven't seen noticeable performance decreases. (BTW @joernroeder I owe you a buncha beer! Thanks so much.)

seubert commented 10 years ago

(FWIW I did see CPU getting demolished when running ~50+ sandboxes with the mem leak fixes, but I am not sure if that would have happened previously or not. Not sure how heavy-handed that gc() call is, processing-wise.)

paulrutter commented 9 years ago

Whats the status on this one? I'm still having memory issues with the latest release (1.3.2).

seubert commented 9 years ago

@paulrutter I haven't touched this in months, but the fixes from https://github.com/joernroeder/sandcastle/tree/memoryLeakFixing were somewhat helpful. We also ended up fixing a lot of bugs in the pool queue logic but I don't recall how much performance impact that had. @cwandrews might have some Hot Tips. I am no longer at the company that did a lot of the fixes so I can no longer see about getting them back into master, unfortunately.

zeldrinn commented 9 years ago

@bcoe, do you have a notion of which of the fixes that have been discussed has been most pragmatic? It looks like the issue still exists as of 1.3.3 and it would be great to try to implement a fix on my end (for my own purposes and for this repo if appropriate).

zeldrinn commented 9 years ago

@seubert @bcoe, I'm also seeing unexpected behavior with respect to the configurable memoryLimitMB when instantiating a new Pool: I'm setting it to 100 MB but all of my sandcastle processes balloon way past that to 700 MB or more. I figured using the memoryLimitMB config would be a crude but easy solution to the heartbeat-related memory leak, either by forced garbage collection or by killing the process outright; is that not the case?

It looks like memoryLimitMB simply sets the node config value of max_old_space_size when running the sandcastle, so wouldn't the process simply die once the memory leak caused it to exceed 100 MB?

haganbt commented 8 years ago

@bcoe - any plans to look at this issue further in the future or is the recommended approach to review the fixes from https://github.com/joernroeder/sandcastle/tree/memoryLeakFixing?

Appreciate any thoughts or advice, this is blocking us from running sandcastle in production.