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

add clear-cache option #215

Closed
wants to merge 1 commit into from
Closed

Conversation

janro1
Copy link

@janro1 janro1 commented Aug 25, 2021

Hi,

the strategy of deleting the lambda function from the require cache leads to huge memory leaks in our case, as delete seems only to remove the cache key, but not the cached object. I guess you could also use something like decache to cleanly remove the lambda function from cache, but I didn't want to introduce a new dependency to your project. Instead I've added an option to skip the deletion of the cache key, as our dev-server restarts after every code change anyway.

@gpotter2
Copy link
Collaborator

This is unexpected, and I don't understand what you mean by

only to remove the cache key, but not the cached object

@janro1
Copy link
Author

janro1 commented Jan 4, 2022

It means that this:

// delete this function from the require.cache to ensure every dependency is refreshed. 
delete require.cache[require.resolve(lambdaPath)];  
lambdaFunc = require(lambdaPath);

removes nothing from the heap. It just allocates more and more memory until the VM memory is full. Libs like decache (see above) try to solve this issue. As we restart our dev-server after every code change anyway, we don't need this behaviour. That's why I introduced this flag.

@gpotter2
Copy link
Collaborator

gpotter2 commented Jan 4, 2022

removes nothing from the heap

NodeJS has a gc.. what do you mean?

Libs like decache (see above) try to solve this issue.

Does it really though?

You can have a look at what decache does: I'm not sure how it differs from what we're doing.
https://github.com/dwyl/decache/blob/main/decache.js#L34-L36
It does have some code to recursively throw away all modules required by the the module, but unsure how that helps us :/
Maybe there's something I'm missing, and a reference to the module is kept somewhere that should also be deleted, if so feel free to point it out :p

@janro1
Copy link
Author

janro1 commented Jan 4, 2022

I just noticed that the code above leads to memory leaks. I have no details how node handles the require cache internally. But GC'ing required modules is probably a dangerous thing. I would not expect this to work.

I remember that I've tested decache when i found the issue and it was working better, but I don't know if it really cleans the cache fully or if the memory leak is just smaller...

@gpotter2
Copy link
Collaborator

gpotter2 commented Jan 4, 2022

I just noticed that the code above leads to memory leaks

What part? This might be useful

@janro1
Copy link
Author

janro1 commented Jan 4, 2022

again, this part

// delete this function from the require.cache to ensure every dependency is refreshed
delete require.cache[require.resolve(lambdaPath)];
lambdaFunc = require(lambdaPath);

@gpotter2
Copy link
Collaborator

gpotter2 commented Jan 4, 2022

This is spinning in circles. Could you provide a reproducible example?
I a do not intend to merge this PR as-is, as it's just a workaround to a potentially bigger problem?

@janro1
Copy link
Author

janro1 commented Jan 4, 2022

You can find more details here: nodejs/node#14569

this is a tiny example:

file run.js

while(true) {
    const file = './func.js';

    delete require.cache[require.resolve(file)];
    const lambdaFunc = require(file);
}

file func.js

var arr = new Array(40 * 1024 * 1024);

module.exports = {
    a: arr
}

and then run node --trace_gc run.js

@gpotter2
Copy link
Collaborator

gpotter2 commented Jan 4, 2022

Thanks !!
I tried implementing something based on decache. Could you try out https://github.com/ashiina/lambda-local/tree/decache and report if it works?

@janro1
Copy link
Author

janro1 commented Jan 5, 2022

I've tried your new decache version, but sadly it does not really improve the situation. If i comment out the newly introduced line: decache(lambdaPath); the leak is gone. Additionally, everything runs much faster with the line commented out as the re-requiring of the lambda function takes (naturally) a lot of time.

@janro1
Copy link
Author

janro1 commented Mar 2, 2022

Hi @ashiina,

I'm closing this PR because I've created my own lib to run lambda functions locally. It uses forks / child processes to avoid the memory leaks and problems discussed in this PR. It lacks probably some features lambdaLocal has, but it works very well for us. Since we don't need to clean the require cache and re-requiring the lambda handler for each request, it also runs like 10 times faster.

@janro1 janro1 closed this Mar 2, 2022
@janro1 janro1 deleted the add-no-clear-cache branch March 2, 2022 17:05
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.

None yet

2 participants