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

fix: Child processes are cleaned up when useChildProcesses is used #1234

Merged
merged 1 commit into from Jun 1, 2021

Conversation

EduardMcfly
Copy link
Contributor

Description

When using useChildProcesses: true, child processes are killed when they finish their execution

Motivation and Context

#1105

How Has This Been Tested?

In the latest version of serverless-offline, using the v12.20.1 version of node and using the task manager, I was able to verify that when the invocation of the function finishes it closes it correctly

I wanted to do unit tests for this but I really don't know what they would be like

@chaddjohnson
Copy link

This could not have come at a better time for me. I was able to test this via

"devDependencies": {
  "serverless-offline": "git://github.com/cosva-lab/serverless-offline#86d0166ae84008f28400fa6bb039f20d5142d992",
  ...
}

and have confirmed this fix indeed works. My system is no longer running out of memory.

@dherault Can we get this merged and released?

@@ -1,7 +1,7 @@
import { resolve } from 'path'
import path from 'path'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you still need to pull in everything from path here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I changed this because according to the eslint rules an adavetence was launched with the resolve of the promise

It doesn't seem like good practice to me to leave the promise resolution under another name, so import the 'path' and don't destructure the object.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah gotcha

Copy link
Collaborator

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @EduardMcfly, great catch 🙇

@pgrzesik pgrzesik changed the title fix: child processes are cleaned up when useChildProcesses is used fix: Child processes are cleaned up when useChildProcesses is used Jun 1, 2021
@pgrzesik pgrzesik merged commit 3fb5b69 into dherault:master Jun 1, 2021
@jaska120
Copy link

@pgrzesik Could a new release be made, which includes this fix please?

@pgrzesik
Copy link
Collaborator

Sure @jaska120 - it just has been released with v7.1.0, sorry for the delay

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

5 participants