mu-semtech / mu-javascript-template

Template for running javascript/express microservices
4 stars 17 forks source link

make boot.sh process PID 1 #53

Open nvdk opened 1 year ago

nvdk commented 1 year ago

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

madnificent commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.