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

make boot.sh process PID 1 #53

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

make boot.sh process PID 1 #53

wants to merge 6 commits into from

Conversation

nvdk
Copy link
Member

@nvdk nvdk commented Jul 10, 2023

small addition to #51 which should be merged first, this makes boot.sh process 0 in the container so that it correctly receives all signals sent to it. this allows it to correctly respond to kill signals sent from docker

@nvdk nvdk marked this pull request as draft July 10, 2023 14:41
we want boot.sh to correctly receive and forwards signals. this commit removes
the double indirection (`CMD` in non array form will already execute the command
in a sh subshell and we wrapped our script in another one).
@madnificent
Copy link
Member

Hi, thanks for the PR.

We've merged master into this one but it seems this only makes it restart faster on development mode as is.

We have added support for a callback function on exit and a default implementation which exits the node process. This makes booting faster but we're not entirely certain about the proposed API. Even without this API I guess it's just a faster random exit.

We have pushed this experimental feature on feature/faster-stop but I'm not sure if/how you can adapt the PR to use that branch instead.

Food for thoughts, suggestions welcome.

@nvdk
Copy link
Member Author

nvdk commented Sep 22, 2023

from reading the docs I think the default should be:

server.close(() => {
    console.log('HTTP server closed because of SIGTERM')
    process.exit();
  })

this should allow express to shutdown gracefully (e.g. stop accepting new requests, but still respond to ongoing requests).

@madnificent madnificent changed the title make boot.sh process 0 make boot.sh process PID 1 Sep 22, 2023
We only exported setExitHandler but that doesn't do much when we don't
import it first.
Here is a new exitHandler based on @nvdk's suggestions and research.  It
doesn't fully behave the way we intend (the last message is not logged)
but does further serve as a proof of concept.
@madnificent
Copy link
Member

I checked node's API but better to use Express's API indeed.

We could supply the server to the shutdown function which would let users call server.close( ... ) or whatever else.

Trying to execute this I did ran into a some issues: the callback function supplied to server.close did not seem to execute (or at least, it can't log anything). Basing myself on the documentation instead of the post, I find app.listen which returns http.server. The docs regarding the close function further point to server.close for the close function.

All of this points to what you suggest, though running the code I've just pushed onto your branch, Shut down complete is not visible on the logs.

This should not be a show-stopper but perhaps there's something we have misunderstood? I have not tried with server[Symbol.asyncDispose]() as mentioned in the last link.

@nvdk nvdk marked this pull request as ready for review October 10, 2023 08:44
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.

2 participants