Incorrect handling of SIGHUP

[axxie]

I think the implementation of handling of SIGHUP is not 100% correct.

The proper handling of SIGHUP is to terminate the application and its children, but nodemon restarts the child instead.

The changes in SIGHUP handling were introduced with PR #1167 (commit 30f999a). This commit introduced the feature to allow restarting child by sending SIGHUP to nodemon process. While the feature itself if fine, the choice of the signal, in my opinion, is not 100% correct. It think it would be better to always use SIGUSR2 for triggering restart. The details are below.

Expected behaviour

nodemon should terminate its child and itself upon receiving SIGHUP signal.

Actual behaviour

nodemon restarts the child.

Steps to reproduce

Use the following application:

const fs = require('fs')

fs.writeFileSync('signals.log', 'Log start\n')

function log (msg) {
    fs.writeFileSync('signals.log', msg + '\n', { flag: 'a' })
}

var counter = 1
setInterval(() => log(counter++), 1000)

With Visual Studio Code

1. Launch VS Code, open folder with the nodemon repository
2. Save the sample code as signals-test.js
3. Open integrated terminal (Ctrl + `)
4. Run the following command:

node bin/nodemon.js signals-handler.js   

5. For convenience, open signals.log in VS Code editor (it automatically reloads log file after changes). Alternatively, run the following command in another terminal:

tail -f signals.log

6. Close the integrated VS Code terminal by clicking the trashcan icon

Notice the updates to signals.log file are restarted and continue from the background instance of the signals-test.js.

With ssh

1. Install and configure ssh server
2. Connect to the machine via ssh. It is ok to connect to localhost for this test.
3. Change dir to the nodemon repository
4. Run the following command in ssh session:

node bin/nodemon.js signals-handler.js   

5. In another terminal also change dir to nodemon repository and run the following command:

tail -f signals.log

6. In yet another terminal kill the ssh client:

pkill -f ssh localhost

Notice the updates to signals.log file are restarted and continue from the background instance of the signals-test.js. Nodemon behaves like nohup utility for applications launched in ssh session.

Environment

nodemon: 2.0.2 (from sources in master)
node -v: v12.14.0
Operating system/terminal environment: Ubuntu 18.04.3 LTS

Analysis

According to man 7 signal, SIGHUP is sent when “Hangup detected on controlling terminal or death of controlling process”. Similar description can be found in Node documentation.

Historically SIGHUP was useless for daemons because they have no controlling terminal. SIGHUP was then reused for triggering the reloading of daemon configs. However, the original handling is still valid and probably required for non-daemon apps, specifically, for applications started in the terminal.

How to test this behavior: run the sample code in the VS code terminal or via ssh directly, without nodemon. Then close the terminal or kill the client connection, just like described in the “Steps to reproduce section above”. Notice that the signals.log file is updated while terminal is open and updates stop when the terminal is closed. This is in the contrast to the launching the same code from under the nodemon.

Proposed fix

I’ve made a PR with one variant of the fix. It contains the following changes:

1. Handle only SIGUSR2 to trigger reload
2. On SIGHUP quit with corresponding error code
3. Also added tests for SIGHUP in separate commit

Since this is a breaking change (though a minor one), I would like to propose alternative solutions:

1. Specify signal for reload as a separate command line and config parameter, keep the current behavior as default if parameter is not specified. The disadvantage of this approach is that with VS Code and ssh the default behavior would be counterintuitive.
2. Add a parameter to ignore reload signal and keep the current behavior as default. Same disadvantage as 1.

Alternatively, the parameter can be used to switch to old (current) behavior.

[remy]

I've not fully digested the change, but the pr you're referring to didn't change functionality, but added a feature that allowed a user to define the restart signal. Otherwise behaviour remained the same.

If the user doesn't want to restart with SIGUSR2 (the default), then they can specify a different signal.

As such, there's no coded handling in nodemon for SIGHUP (or at least that I remember - I'm on my phone ATM so haven't done a detailed check).

But with that in mind, doesn't it render the issue as void?

[axxie]

I've not fully digested the change, but the pr you're referring to didn't change functionality, but added a feature that allowed a user to define the restart signal. Otherwise behaviour remained the same.

As I can see, the line 140 was added in the pr #1167:

nodemon/lib/nodemon.js

Lines 138 to 143 in f0c5353

	if (!config.required) {
	const restartSignal = config.options.signal === 'SIGUSR2' ? 'SIGHUP' : 'SIGUSR2';
	process.on(restartSignal, nodemon.restart);
	utils.log.detail((config.options.restartable ? 'or ' : '') + 'send ' +
	restartSignal + ' to ' + process.pid + ' to restart');
	}

If I understand it correctly, this is exactly handling of the restart signal.

The --signal command-line option existed even before that commit, but it was used only to specify the signal to send to children for restart. With pr #1167 it now does two things at once: specifies the signal to send to children and implicitly changes what signal to handle to restart itself. However, the documentation only mentions the first meaning of the option.

If the user doesn't want to restart with SIGUSR2 (the default), then they can specify a different signal.

In fact, the default is that SIGHUP is handled for manual restarting, and SIGUSR2 is sent to children. I was also confused by it, but look at the line 139.

You are right that if user supplies --signal it solves the issue. However, in my opinion at least something else has to be fixed: documentation, logic of changing restart signal or maybe defaults.

[axxie]

May be it would be enough to just hardcode restart signal as SIGUSR2, regardless of what signal is sent to children.

[stale]

This issue has been automatically marked as idle and stale because it hasn't had any recent activity. It will be automtically closed if no further activity occurs. If you think this is wrong, or the problem still persists, just pop a reply in the comments and @remy will (try!) to follow up.
Thank you for contributing <3

[axxie]

I don't think it should be marked stale

[stale]

This issue has been automatically marked as idle and stale because it hasn't had any recent activity. It will be automtically closed if no further activity occurs. If you think this is wrong, or the problem still persists, just pop a reply in the comments and @remy will (try!) to follow up.
Thank you for contributing <3

[axxie]

Still not stale

[remy]

Since I still don't understand this PR and issue, I'm going to take the questions one at a time.

Firstly:

Incorrect handling of SIGHUP

Do you mean a) the signal sent from nodemon to the subprocess, or b) the signal that a user can send to nodemon?

[axxie]

The answer is:
b) the signal that a user can send to nodemon

Normal console tools (not daemons) should terminate on SIGHUP. And SIGHUP is what VS Code and ssh send on closing the terminal.

[stale]

This issue has been automatically marked as idle and stale because it hasn't had any recent activity. It will be automtically closed if no further activity occurs. If you think this is wrong, or the problem still persists, just pop a reply in the comments and @remy will (try!) to follow up.
Thank you for contributing <3

[axxie]

Still not stale

[stale]

This issue has been automatically marked as idle and stale because it hasn't had any recent activity. It will be automtically closed if no further activity occurs. If you think this is wrong, or the problem still persists, just pop a reply in the comments and @remy will (try!) to follow up.
Thank you for contributing <3

[axxie]

Still not stale

[TheLudd]

I found this issue because it also gives me trouble.
I use tmux when developing, and in some windows I run nodemon to run my web server in dev mode.
When executing :kill-session in tmux it sends SIGHUP to all processes. What then happens is that nodemon restarts and the window that nodemon is in is destroyed. But my server is still running.

So now I have an "invisible" process running and if I try to start up a new dev session it says that the port I use is already in use.

So what should happen in my specific case would be that
1 tmux sends SIGHUP to nodemon
2 nodemon sends SIGHUP to my application that can listen for this and stop itself
3 nodemon exists gracefully

PS stalebot is a bit aggressive IMHO.

[concatime]

I’ve opened a new issue to discuss on proper signal handling.

[axxie]

@remy, if you have more questions I would be glad to answer

[stale]

This issue has been automatically marked as idle and stale because it hasn't had any recent activity. It will be automtically closed if no further activity occurs. If you think this is wrong, or the problem still persists, just pop a reply in the comments and @remy will (try!) to follow up.
Thank you for contributing <3

[axxie]

Still not stale

[remy]

I'm going to close this issue to continue discussion at #1705 - I'm close to getting my head around the potential problems and possible solution.

I drew out a table of how nodemon and the subprocess is handling the signals and I think the way forward is to forward SIGHUP (but not sure how or whether nodemon should exit - I'll continue this thought on #1705) and to use SIGUSR2 as the reload signal on nodemon (as it is currently broken). As to whether it's a breaking change, technically, I think it is. I'm just not keen on doing a major bump for potentially an edge case.

I'm also going to capture the points that @axxie has already brought up (the --signal switching the reload signal on nodemon, documentation and so on).