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

Serve command doesn't detect nodeJS app process exiting #9239

Closed
joshribakoff-sm opened this issue Mar 8, 2022 · 19 comments
Closed

Serve command doesn't detect nodeJS app process exiting #9239

joshribakoff-sm opened this issue Mar 8, 2022 · 19 comments
Assignees
Labels
outdated scope: node Issues related to Node, Express, NestJS support for Nx type: bug

Comments

@joshribakoff-sm
Copy link

joshribakoff-sm commented Mar 8, 2022

Current Behavior

Serve a nodeJS program that does process.exit(1).

It does not output anything and the user has to rely on other observations to deduce that their code exited. In fact editing the code still creates normal webpack output as if the process was reloaded, but the process is dead and is never restarted. The developer has to kill the serve command manually and restart it manually to restore the nodeJS app's process.

Expected Behavior

The serve command should output a message saying that my code exited, and what the exit code was. Editing any code should start the process back up [if it was dead]. I expect making an edit to the code and webpack "reloading" to restart a dead nodeJS program automatically.

Steps to Reproduce

Create a node app that does this

process.exit(1)

Environment

Mac OS

This is also a duplicate of #4054 it appears that issue was closed despite the linked fix being totally tangential to the root issue

@joshribakoff-sm joshribakoff-sm changed the title Serve command doesn't detect child process exiting Serve command doesn't detect nodeJS app process exiting Mar 8, 2022
@AgentEnder AgentEnder added the scope: node Issues related to Node, Express, NestJS support for Nx label Mar 11, 2022
@hcharley
Copy link

When you say:

Editing any code should start the process back up [if it was dead]

I feel like this should only be true if the --watch argument is true (which is the default.

In the case where --watch is false, I think it should notify the user as you say, and then exit.

This will help those of us developing jobs (as opposed to always on servers) to chain subsequent commands/notifications. Right now, it seems like the only choice is to build and run.

@joshribakoff-sm
Copy link
Author

joshribakoff-sm commented Mar 23, 2022

I would agree 🎉

Side note: I am in fact writing a job that runs once and exits, but I want to develop with "watch" mode. The use case is to re-run the job on any code change, including re-attempt the job after fixing a crash in a previous job run :) If the job completes, I would want output in the terminal as such, and if it exits I would also want feedback in the terminal. In both cases, I'd want it to start the job again on any code change (but only if watch mode is on)

@hcharley
Copy link

That's helpful context in your side note @joshribakoff-sm. It made me wonder...

I like that watch auto reloads, but I wish that I could have the option to let the job finish before reloading, or allow for specific Grace periods instead of 'instantly'.

Maybe this already exists and I've forgotten or don't know about it.

I think in the past, I've tried to delay on a SIGINT in my code, but what happens (I have to double check) is that the next job starts and then they are both running.

I probably just could write a better, faster SIGINT cleanup, but it's still my uninformed impression that the grace period is 0 and isn't configurable.

Anyways, not sure if this wondering is relevant.

@hcharley
Copy link

BTW, the way I am getting the functionality I need is to use an expect script:

#!/usr/bin/expect -f

set timeout -1

puts "Running with args: $argv\r"

spawn tools/scripts/run-my-app.sh $argv

expect "[My app] is exiting with"

send "done\r"

exit 0

And in run-my-app.sh:

#!/bin/bash

pnpm run -- nx run my-api:serve \
  --port=12012

@aqemi
Copy link

aqemi commented May 6, 2022

It's not only process.exit(), it's even not exiting on empty script (implicit node shutdown). Seems like the watch flag doesn't change any behavior.

@github-actions
Copy link

github-actions bot commented Nov 3, 2022

This issue has been automatically marked as stale because it hasn't had any recent activity. It will be closed in 14 days if no further activity occurs.
If we missed this issue please reply to keep it active.
Thanks for being a part of the Nx community! 🙏

@github-actions github-actions bot added the stale label Nov 3, 2022
@joshribakoff-sm
Copy link
Author

not stale 😬

@github-actions github-actions bot removed the stale label Nov 4, 2022
@firxworx
Copy link

firxworx commented Nov 4, 2022

Yeah just hit this +1

@kaifresh
Copy link

Not stale! Please fix

@eljou
Copy link

eljou commented Dec 29, 2022

Yes please fix

@ValiDraganescu
Copy link

Yes please fix Yes please fix :)

@PiotrOwsiak
Copy link

First reported in 2020 (#4054)
then again mid 2021 (#5726)
and still no fix on the horizon?

@PiotrOwsiak
Copy link

Quick and dirty solution? Kill the parent process:

process.kill(process.ppid, 'SIGTERM')

Beware this makes nx report an error:
NX Running target "xxxxx:serve" failed
Failed tasks:
...
error Command failed with exit code 1.

Copy link

github-actions bot commented Jan 8, 2024

This issue has been automatically marked as stale because it hasn't had any recent activity. It will be closed in 14 days if no further activity occurs.
If we missed this issue please reply to keep it active.
Thanks for being a part of the Nx community! 🙏

@github-actions github-actions bot added the stale label Jan 8, 2024
@joshribakoff-sm
Copy link
Author

Keep

@github-actions github-actions bot removed the stale label Jan 9, 2024
@simonmallet
Copy link

Quick and dirty solution? Kill the parent process:

process.kill(process.ppid, 'SIGTERM')

Beware this makes nx report an error: NX Running target "xxxxx:serve" failed Failed tasks: ... error Command failed with exit code 1.

I have spent the whole morning trying to get my script with "executor": "@nx/js:node", to exit with an error.
Using watch: false, it DOES NOT WORK. It always returns a success NX Successfully ran target.
But when using watch: true, it exits with an error NX Process exited with code 1, waiting for changes to restart...

I run scripts in azure pipelines which I NEED to have watch: false.
So right now the pipeline shows it ran successfully becauase of this error instead of throwing an error like it should.
The suggestion to use process.kill(process.ppid, 'SIGTERM') works... so this will be my workaround.

Can you make the executor propagate the error when watch is set to false so we can use process.exit(1) instead of process.kill ?

@fabricio-getaway
Copy link

fabricio-getaway commented Mar 19, 2024

I was going to create an issue but I've just found this one. Here's more info and a repo to duplicate the issue:

Repo:

Link: repo to duplicate the issue

The app in the referenced repository throws a simulated error that should result in an exit code of 1.
The serve target is set to "watch: false" for production.

Steps:

  1. Run npx nx serve my-node-app --prod

This is the only step required. Despite the error being displayed, the output still reads:

NX Successfully ran target serve for project my-node-app

This implies that the host will never know if the application encountered an error during execution, as the Node executor incorrectly returns an exit code of 0 instead of 1.

Additional Information:

Nx report

Node   : 20.11.0
OS     : win32-x64
npm    : 10.2.4

nx                 : 18.1.2
@nx/js             : 18.1.2
@nx/jest           : 18.1.2
@nx/linter         : 18.1.2
@nx/eslint         : 18.1.2
@nx/workspace      : 18.1.2
@nx/devkit         : 18.1.2
@nx/esbuild        : 18.1.2
@nx/eslint-plugin  : 18.1.2
@nx/node           : 18.1.2
@nrwl/tao          : 18.1.2
typescript         : 5.3.3

Additional Information

Although the issue is not observable when 'watch' is set to true, using such a configuration is not suitable for a production environment.

I think these are the relevant lines:

task.childProcess.off('data', handleStdErr);
if (options.watch && !task.killed) {
logger.info(
`NX Process exited with code ${code}, waiting for changes to restart...`
);
}
if (!options.watch) done();

Basically, if the task is not being watched, it is signaled as done() but its exit code is never used.

@jaysoo
Copy link
Member

jaysoo commented Apr 9, 2024

This is fixed here #22705

@jaysoo jaysoo closed this as completed Apr 9, 2024
Copy link

This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated scope: node Issues related to Node, Express, NestJS support for Nx type: bug
Projects
None yet
Development

No branches or pull requests