node-gradle / gradle-node-plugin

Gradle plugin for integrating NodeJS in your build. :rocket:
Apache License 2.0
606 stars 120 forks source link

Gradle process stop doesn't lead to node process stop #65

Open imanushin opened 4 years ago

imanushin commented 4 years ago

Short: stopping gradle execution on Windows from command line by pressing Ctrl-C doesn't lead to stopping node.exe subprocess on Windows.

Steps to reproduce:

  1. Create project with webpack
  2. Configure start at package.json script via:
    "scripts": {
    "start": "webpack-dev-server --open --config webpack.dev.js"
    }
  3. Create npmStart task via gradle kts script:
tasks {
    val npmStart by registering(NpmTask::class) {
        /*dependent tasks are skipped for simplification*/
        setArgs(listOf("run", "start"))
    }
}
  1. Start local node server via .\gradlew.bat :npmStart
  2. Wait for process start (web browser opens)
  3. Press Ctrl-C to stop node.exe execution

Expected result: All child processes under gradle will be stopped.

Actual result: Node.exe process remains. And this process starts to be zombie, e.g. process without parent.

Hint: probably (I'm not sure, this require testing), NpmTask should be inherited from Exec, not from DefaultTask. It means that args field will be inherited from Exec task, so it will be fully configurable via user. However next Gradle tool will be responsible to stop all child tasks.

deepy commented 4 years ago

I suspect inherting from Exec won't be enough to solve this as there's an open issue that matches this https://github.com/gradle/gradle/issues/7603.

To solve it we probably need to start npm through node rather than relying on npm.cmd

imanushin commented 4 years ago

Agree, that problem won't be fixed just after task inheritance change. However after this change we can just wait for fix from Gradle team. E.g. for current moment issue will exist even if Gradle team fixes Exec task behavior.

deepy commented 4 years ago

I'm pretty sure they both share the same issue and will be fixed at the same time, but if it's only fixed in Exec it's probably going to be easy to also get that fix for project.exec.

imanushin commented 4 years ago

@deepy , as @philiparvidsson said here:

my bad - I edited the original post to clarify! You are correct in that it only affects bat-files.

Therefore, I can advice the following solutions:

deepy commented 4 years ago

Pretty sure that'd have the same issue. https://github.com/gradle/gradle/issues/7603#issuecomment-448963470

Thank you for the elaborate investigation! So we'd see similar issues if a program forked with exec() forked another program on Windows. cmd is just a common example of this situation.

atali commented 4 years ago

Any update on that issue ?

deepy commented 4 years ago

This really depends on the linked Gradle issue, which sounds like it should be easy to do with Java 9+.

The best way to get this fixed would be to send a PR to Gradle

jGleitz commented 4 years ago

Hi! The referenced Gradle issue only relates to Windows. However, I am also experiencing this bug on Linux: When I stop the Gradle task serving my docz site, the node processes are not killed. Does this mean that the Gradle issue also applies to Linux or might there be something that can be fixed in this plugin?

deepy commented 4 years ago

I might be wrong here, but I was under the impression that it applied to all operating systems. At least I've seen similar issues on macOS and Linux

jGleitz commented 4 years ago

Hm, seems you are right. The original issue stated that

NOTE 1: This seems to affect Windows only!

But the latest commenter reproduced the issue on macOS. So it will likely be the same on Linux.

luxmeter commented 3 years ago

Regarding the platform: I have the same issue on linux with gradle and webpack.

thSoft commented 3 years ago

I also managed to reproduce this both on macOS and Linux. As a workaround, I kill the started node process (based on the port) besides stopping the Gradle daemon.

buergerj commented 3 years ago

I can provide information that may help to solve this issue. We also encountered this problem. We use the Gradle Node plugin with yarn to run vite to serve a frontend during development time. Maybe, the following information does also apply to npm. In our tests, we only checked Linux so far, but the information may help to check this also for other platforms. We also encountered the described issue, but strangely, one colleague did not encounter the issue. We compared a plethora of Gradle, Node, etc. related settings and found no difference. However, we found out the following: To start vite, yarn does not do this directly but by utilizing a shell. Node uses a hard-coded path for that, for Linux-like systems this is /bin/sh (a colleague checked that by looking into the sources).

As you may know, sh is gone since long, and today's systems provide /bin/sh only as a link to a sh-compatible successor. On Ubuntu, Mint and probably other Debian-based systems, this typically is Dash. On Arch Linux, this typically is Bash.

Dash creates a child process for the above mentioned call. Killing the host process (via ^C, "Stop" button in your IDE, or else) does not kill the child process, so node (ultimately vite) continues to run.

On the contrary, Bash does not create a child process and replaces itself by the program to run (the node call). You can tell that by the bash process vanishing from the system's process list. Thus, a kill does actually kill the node process.

This issue can be solved by having Bash installed and explicitly setting it as the shell to use, like so: yarn config set "script-shell" "/usr/bin/bash". npm has such a setting, too.

So, people affected by this issue may try the above mentioned or at least check which shell is used as the link target of sh.

JD10NN3 commented 3 years ago

Any update on this one ?

justfortherec commented 3 years ago

Thanks @buergerj, for the workaround. I can confirm that it works on my Linux machine (with the slight change to use the path /bin/bash where bash is available on my system).

deepy commented 3 years ago

Just a reminder that this should be fixed in Gradle itself

AndreasNasman commented 2 years ago

Thanks @buergerj for the workaround, it helped me solve a weeklong issue! 🙏I managed to figure out Dash was the likely cause but didn't know it spawned child process and that Bash doesn't.

mfwgenerics commented 1 year ago

Another thanks to @buergerj for the config workaround. To keep the installation more self-contained and avoid a separate yarn config set command I opted to:

  1. Populate a linux.yarnrc file into the project directory. Example
  2. Pass --use-yarnrc=linux.yarnrc as an argument to yarn. This must come before start in the argument list. Example
rafeememon commented 1 year ago

Here is my workaround (on Windows):

For running tasks during development outside of the normal build flow (such as npm start, npm install) I use a wrapper batch file that directly calls the downloaded installation of node/npm, skipping Gradle and thus the wrapped process issue. A couple additional benefits:

  1. Faster startup time since Gradle configuration is skipped.
  2. Easier to call ad-hoc npm commands.
  3. No custom user configs necessary; can be self-contained to the project.

A couple downsides:

  1. Need to be extra sure that the NpmTask dependencies in the build are wired up correctly to be executed as part of a "real" build, but this is probably already the case in a well-behaved build. For me, this is really just npm install -> npm run build.
  2. Need to call npm.bat instead of npm. I'm happy with tab completion, but there are probably better solutions.

The creation of the wrapper batch file can be automated with a Gradle task:

def npmShortcutPath = 'npm.bat'

task npmShortcut {
  dependsOn nodeSetup
  inputs.property 'nodeVersion', nodeVersion
  outputs.file npmShortcutPath
  doLast {
    def nodeDir = nodeSetup.nodeDir.get()
    def contents = "@echo off\n${nodeDir}\\nodevars.bat && npm.cmd %*\n"
    file(npmShortcutPath).text = contents
  }
}

tasks.named('npmSetup') {
 finalizedBy npmShortcut
}

Add npm.bat to .gitignore. Something similar can be done for npx as well.

amir1376 commented 1 year ago

any updates ?

deepy commented 1 year ago

@amir1376 this is https://github.com/gradle/gradle/issues/7603

PeredurOmega commented 1 year ago

I also recently encountered this issue and I made a custom workaround based on Gradle Shared Build Services (https://docs.gradle.org/current/userguide/build_services.html). The idea is that the service is attached to npm tasks and kills them properly (i.e. also killing dependents processes) in case the Gradle task is stopped. If it may be of any use to someone else, I made a small Gradle plugin for it https://github.com/PeredurOmega/GradleNpmPlugin mainly resolving this issue + automatically defining tasks for scripts defined in package.json.

virtualdogbert commented 1 year ago

Three years later and this is still an issue... @buergerj how do I try what you're talking about, from within my build.gradle for npm? Are there any other workarounds that involve just changing/setting something in the build.gradle?

deepy commented 1 year ago

I get how frustrating this can be, but this is really an issue that needs to be solved upstreams as this is caused by a bug in Gradle If you're comfortable working on solving the issue there's https://github.com/gradle/gradle/pull/18756 which needs a little extra work, otherwise I'll also be happy to include workarounds in the plugin provided they can be tested and toggled by a feature flag

virtualdogbert commented 1 year ago

I get that it's an upstream issue and sorry if I came off annoyed with anyone here. Strange and more people aren't complaining about this issue as it affects both Windows and Linux, and I would have to guess more than just this plugin, which I love because the less I have to deal with frontend tools the better.

I was just hoping I could get more details on how to get the workaround talked about, and if I could set up that workaround in my build.gradle for npm. If the workaround involved actually installing node/npm that would kind of defeat the purpose of the plugin for me, then my workaround would just be to have a process window up, and kill the process manually when I'm done.

I would not be comfortable working on it as I don't have a Windows machine(haven't for years), so I could only test the Linux path at best.

vibdev commented 8 months ago

This worked for me! https://github.com/node-gradle/gradle-node-plugin/issues/65#issuecomment-1442107492

deepy commented 8 months ago

Just want to add that the underlying issue for this can be reproduced on Linux and macOS as well, but due to the way node is packaged it will only affect Windows users Giving a 👍 on the issue in the gradle repository is a good way to signal that you're affected (and helps with prioritization)

marlow-fawn commented 6 months ago

Seems like this could be resolved at least partially by #228 if it implements autocloseable, right? This is what https://github.com/node-gradle/gradle-node-plugin/issues/65#issuecomment-1442107492 does.

deepy commented 6 months ago

Unfortunately the build services work has stalled due to https://github.com/gradle/gradle/issues/17559 🥲 But the underlying Gradle issue would still affect us no matter which solution is picked, the way to work around it would be to implement the process tracking in the plugin itself and that's a too large scope for the current team