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: killing the process hangs issue #169

Closed
wants to merge 3 commits into from

Conversation

helio-frota
Copy link
Member

Related to #120

2023-02-04_08-36

@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2023

Codecov Report

Base: 92.36% // Head: 92.36% // No change to project coverage 👍

Coverage data is based on head (14edff9) compared to base (64a41b2).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #169   +/-   ##
=======================================
  Coverage   92.36%   92.36%           
=======================================
  Files           9        9           
  Lines         262      262           
=======================================
  Hits          242      242           
  Misses         20       20           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@helio-frota
Copy link
Member Author

Steps to reproduce:

  1. create a function
  2. change the dependency to use this:
"faas-js-runtime": "github:helio-frota/faas-js-runtime#replace-death"
  1. Run the function
  2. Try to access the function via browser (I'm using our electron example .. npm install , npm start)
  3. kill the function with control + c

@helio-frota
Copy link
Member Author

Video:

foo-2023-02-04_09.37.53.mp4

@helio-frota
Copy link
Member Author

After merge the changes from main branch I started to see this in CI

2023-02-11_10-12

I'm using Node 18 maybe is this something related to package-lock.json ?

@helio-frota
Copy link
Member Author

I'm using Node 18 maybe is this something related to package-lock.json ?

Changed to node 16x , deleted and re-generated package-lock.json, updated the PR and now the CI is working

@lholmquist
Copy link
Member

@lance was there a reason for choosing the death module?

@lance
Copy link
Member

lance commented Feb 14, 2023

@lance was there a reason for choosing the death module?

Nothing other than, "it seems to work as expected" 🤣

@@ -40,7 +40,7 @@ async function runServer(file) {
console.error(code);
throw TypeError(`Cannot find Invokable function 'handle' in ${code}`);
}
ON_DEATH(_ => {
nodeCleanup(_ => {
server.close();
Copy link
Member

Choose a reason for hiding this comment

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

I think the only thing that needs to be added is a process.exit(0) after the server.close instead of changing the module.

Copy link
Member

Choose a reason for hiding this comment

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

@helio-frota can you give this a try in your test scenario?

Copy link
Member Author

Choose a reason for hiding this comment

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

@lholmquist @lance that works thanks!

Copy link
Member Author

@helio-frota helio-frota Feb 14, 2023

Choose a reason for hiding this comment

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

interesting it has a little bit different behavior we need 2 control+c but works.. [ maybe a delay thing happening don't know ]
I saw that other node.js tools also need 2 control+c , sometimes

@helio-frota
Copy link
Member Author

PR sent #172

@helio-frota helio-frota deleted the replace-death branch February 14, 2023 16:53
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.

4 participants