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

POC for externally triggering cleanup of lambda functions #1093

Merged
merged 1 commit into from Apr 13, 2022

Conversation

kelchm
Copy link
Contributor

@kelchm kelchm commented Sep 5, 2020

Based on previous discussions with @dl748 around #1090, I've implemented this proof of concept for how serverless-offline might allow other plugins to trigger a lambda cleanup for hot reloading.

Even without integration directly within something like serverless-webpack, this enables a custom plugin to hook the relevant event and then invoke the offline:functionsUpdated command that this PR adds (example).

Like with #1090, this still relies on useWorkerThreads: true and allowCache: true for this to actually work as expected. I'm also not really happy with the naming conventions here, please feel free to share any ideas if you have them.

@dherault
Copy link
Owner

@kelchm great work so far, I'm ready to merge. How could this be improved further?

@kelchm
Copy link
Contributor Author

kelchm commented Oct 26, 2020

@dherault, thanks! It's a stupid hack but I think it is a good option to have available, particularly for large projects.

I unfortunately don't have much bandwidth to spare at the moment, but I'm happy to loop back on this in a week or two to get it polished up if you're onboard with the approach. Beyond adding documentation and test coverage, is there anything specific that you would like to see changed or improved?

EDIT: Sorry -- replied before I had any coffee. If you're happy with this as is, feel free to merge. I can still followup with some documentation additions in the future as my time allows.

@kelchm kelchm marked this pull request as ready for review October 26, 2020 15:04
@dl748
Copy link
Contributor

dl748 commented Oct 27, 2020

This is a good first step, i think we should add the ability for the plugins to "announce" changed modules, that way only those modules can reload rather than every thing. Default (if no modules are provided) reload everything.

@dherault
Copy link
Owner

@kelchm I think it needs more polishing too, but the work done is good enough as is. If you cannot do more work in the following weeks I'll merge anyway. 😎
@dl748 Great feature proposal ✌️

@francisu
Copy link
Contributor

francisu commented Nov 2, 2020

@dherault I would love to see this go in as soon as possible. The serverless-webpack, allowCache is a very common use case as there are many lambda functions that depend on keeping globals alive (which allowCache emulates).

@dgurns
Copy link

dgurns commented Apr 29, 2021

Just bumping this. I'm using serverless-webpack 5.4.2, serverless-offline 6.9.0, and have allowCache: true to prevent the memory leak issue in #1119. Requests are back to ~50ms as opposed to ~1000ms.

However -- hot reload is not working. Is this PR still valid and a possible fix? If so would be great to get that working again.

@james-relyea tagging you in case this is related/helpful to the other thread in #1119. Thanks for getting this project moving again, very exciting to see new releases coming out.

@mitch-byb
Copy link

Bumping this, great proposal and solution - would love to see implemented.

Forked for now to allow us to cache while picking up changes in serverless-webpack compilation, but eager to move back to master.

@CorruptedHeart
Copy link

Any chance of this being merged in? Or what work is still required?

@dherault
Copy link
Owner

Ok let's go! I'm back on the project btw.

@dherault dherault merged commit a5bd9da into dherault:master Apr 13, 2022
@dherault
Copy link
Owner

Published in v8.6.0 (in a few hours)

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

7 participants