Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Interop with nodemon --exec and ncc run #253

Closed
adamelliotfields opened this issue Jan 29, 2019 · 5 comments
Closed

Interop with nodemon --exec and ncc run #253

adamelliotfields opened this issue Jan 29, 2019 · 5 comments

Comments

@adamelliotfields
Copy link

Nodemon sends the SIGUSR2 signal when a watched file changes, which can be handled in your app to gracefully shutdown.

Since NCC does not listen for SIGUSR2, it will error:

Error: Application at src/index.js is already running or didn't cleanup after previous run.To manually clear the last runbuild, try running "rm -rf /tmp/d41d8cd98f00b204e9800998ecf8427e".
[nodemon] app crashed - waiting for file changes before starting...

In #228, listeners for SIGINT and SIGTERM were added. I'd like to add a listener for SIGUSR2 as well (I've tested this locally and it resolves the issue).

Note that ncc run --watch won't work for me because ps.kill() will send a SIGTERM signal to my app, which will cause the process to exit, which stops NCC.

Minimal Reproducible Example

Copy the following files, run npm install, and then npm run start:ncc. While the app is running, make a change to src/index.js and observe the results.

Run npm run start:nodemon and make a change to observe the desired result.

package.json

{
  "private": true,
  "scripts": {
    "start:ncc": "nodemon --exec \"ncc run\" src/index.js",
    "start:nodemon": "nodemon src/index.js"
  },
  "dependencies": {
    "express": "^4"
  },
  "devDependencies": {
    "@zeit/ncc": "^0",
    "nodemon": "^1"
  }
}

src/index.js

const express = require('express');
const http = require('http');

const app = express();

app.get('/', (req, res) => res.status(200).json({
  message: http.STATUS_CODES[200],
  statusCode: 200,
}));

const server = http.createServer(app);

['SIGINT', 'SIGTERM', 'SIGUSR2'].forEach((signal) => {
  process.on(signal, () => {
    console.log('Shutting down...');
    server.close(() => {
      process.exit(0);
    });
  });
});

server.listen({ host: 'localhost', port: 8080 }, () => {
  console.log('Listening on http://localhost:8080...');
});
@guybedford
Copy link
Contributor

Thanks for sharing such a clear description here @adamelliotfields.

We should definitely support the SIGUSR2 signal as expected (as well as any others we may have missed).

As for the run --watch support, this sounds like a bug as well. It should be possible to distinguish between a signal caused by the process itself or a signal from the parent. Possibly with some tracking state. I will look into this when I can.

@adamelliotfields
Copy link
Author

adamelliotfields commented Jan 29, 2019

Thanks for the quick reply, @guybedford!

Adding process.on("SIGUSR2", exit); to cli.js will be a quick fix.

I'll play around with cli.js locally and see if I can get ncc run to not exit when the child process exits. I think it's the ps.on("exit") call.

Per the docs:

The 'exit' event is emitted after the child process ends. If the process exited, code is the final exit code of the process, otherwise null. If the process terminated due to receipt of a signal, signal is the string name of the signal, otherwise null. One of the two will always be non-null.

This works (not sure if this is the direction you had in mind, or if you want to build on top of it):

function exit (code) {
  require("rimraf").sync(outDir);
  // if the child process calls process.exit() on SIGTERM, don't exit
  if (code === 0) return;
  process.exit();
}
// code will either be a number (exit code) or a string (signal)
ps.on("exit", (code) => {
  exit(code);
});
process.on("SIGTERM", exit);
process.on("SIGINT", exit);
process.on("SIGUSR2", exit);

This was referenced Jan 31, 2019
@guybedford
Copy link
Contributor

Thanks for the suggestions @adamelliotfields.

I started looking into the run --watch case, but get the impression there will be other edge cases that would also need to be managed here. For this reason I've posted #259 to throw on this case for now as unsupported, but we can certainly come back around on it as a feature.

@guybedford
Copy link
Contributor

Closing this for now, let's track a separate issue for run --watch as a feature if you're still after this @adamelliotfields, and please report if you notice anything else here.

@adamelliotfields
Copy link
Author

Sounds good @guybedford. For now, Nodemon works for me. --watch would be a nice to have, but not a show-stopper.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants