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

Is there any change to optimize the processing time? #25

Open
debuggy opened this issue Aug 6, 2018 · 12 comments
Open

Is there any change to optimize the processing time? #25

debuggy opened this issue Aug 6, 2018 · 12 comments
Labels
💵 Funded on Issuehunt This issue has been funded on Issuehunt enhancement help wanted

Comments

@debuggy
Copy link

debuggy commented Aug 6, 2018

Issuehunt badges

I need to kill the process tree in 5 seconds. However, I found that the code will run almost 10+ seconds before returned. Any suggestions? Thanks.

const fkill = require('fkill');
...
await fkill(pid, {"force": true, "tree": true}); // It takes longer than expected

There is a $8.00 open bounty on this issue. Add more on Issuehunt.

@sindresorhus
Copy link
Owner

That is very slow indeed. What operating system and Node.js version are you on?

@debuggy
Copy link
Author

debuggy commented Aug 7, 2018

@sindresorhus win10 + nodejs 8.0.0

@sindresorhus
Copy link
Owner

Then probably not. fkill is just using the built-in taskkill command to kill processes on Windows. https://github.com/sindresorhus/taskkill

@debuggy
Copy link
Author

debuggy commented Aug 7, 2018

ok, thanks anyway

@MarkTiedemann
Copy link

MarkTiedemann commented Aug 7, 2018

@debuggy Are those processes doing some kind of I/O by any chance? taskkill should be fast, but you might be interacting with device drivers that don't properly handle the cancellation of outstanding I/O requests, for example.

EDIT: Process Explorer or Process Monitor might help you debug this issue.

@debuggy
Copy link
Author

debuggy commented Aug 8, 2018

@MarkTiedemann Thanks for your kind reply, actually I have tackled this issue by using tree-kill ...

It also executes taskkill command in win32 platform and somehow runs faster in my senario.

@MarkTiedemann
Copy link

@debuggy Since both tree-kill and fkill execute taskkill /pid $pid /t(ree) /f(orce), there should not be any significant performance difference.

Can you perhaps share a minimal reproduction for this case? I'd be happy to help figure out why this discrepancy occurs.

@mugiseyebrows
Copy link

mugiseyebrows commented Oct 3, 2018

In my case fkill was running painfully slow (+40s), and I figured out its because it uses process-exist which uses ps-list which uses tasklist and tasklist was the one who is slow, and also was loading cpu significantly (WMI Provider Host in taskmanager C:\Windows\System32\wbem\WmiPrvSE.exe). I was trying to reproduce this issue in another project and failed, then I removed node_modules folder and package.lock and reinstalled all packages and problem went away, it only took 0.5-2s to run tasklist now which is slow but bearable.
Also I figured it's not neccessary to check if process exists before trying to taskkill it, since taskkill return error code if it cant find specified process. I wish there was an windows-only option like faster or quick-and-dirty to ommit tasklist (processExist) call. Should I write a PR maybe?

@MarkTiedemann
Copy link

@mugiseyebrows The speedup you got from reinstalling was probably related to a recent performance issue in tasklist that has been fixed.

If you want to write a PR that properly handles taskkill exit codes, that would be great. :)

@MarkTiedemann
Copy link

MarkTiedemann commented Oct 17, 2018

@mugiseyebrows ps-list@v6.0.0 just got ~5x faster on Windows. Is it more bearable for your use case now?


Since you only want to kill a single process, there's a huge overhead in process-exists which gets a list of all running processes to determine whether a single process is running.

To mitigate this overhead, we could write a Node native addon (hard to maintain) or build a simple binary which could be executed from Node which only checks whether a single process is running.

For example, from the binary, we could call the OpenProcess function with PROCESS_QUERY_INFORMATION rights, then pass the process handle to the GetExitCodeProcess function and check whether it returns STILL_ACTIVE. (Maybe there are different, better approaches - I haven't done any research yet. This is just a first idea.)

PS: If I have time in the upcoming days, I might work on this. :)

@MarkTiedemann
Copy link

MarkTiedemann commented Oct 17, 2018

@mugiseyebrows I checked the exit codes for taskkill.

0: Success
1: Access Denied
128: Not Found

0 doesn't actually guarantee that the process was killed; it only guarantees that a "termination signal was sent" (which usually means that the process was not terminated immediately, which usually means that it won't ever terminate in response to this, but more reasearch and experiments are required here).

Also, there might be other exit codes in edge cases (see: Windows system errors codes).

Also, when killing multiple processes with taskkill, if one of them fails, the exit code will indicate failure.

λ taskkill /pid 123456 /pid 3000 /f
ERROR: The process "123456" not found.
SUCCESS: The process with PID 3000 has been terminated.

λ echo %errorlevel%
128

Also, when multiple failures occur, the exit code 128 takes priority over 1.

λ taskkill /pid 123456 /pid 0
ERROR: The process "123456" not found.
ERROR: The process with PID 0 could not be terminated.
Reason: This is critical system process. Taskkill cannot end this process.

λ echo %errorlevel%
128

Also, you cannot reliably parse the error message strings (e.g. ERROR: The process "xyz" not found.) for determining the error reason since those are language-specific, e.g. on a German or Spanish Windows installation, these are different.

So yeah, this is a huge mess. :)

In conclusion, I think taskkill should be improved, if possible, or replaced by a less messy alternative, and if we can improve it, then fkill doesn't need to check whether processes exist when it is executed on Windows since the return values should indicate what succeeded and what failed and why it failed.

@IssueHuntBot
Copy link

@0maxxam0 has funded $8.00 to this issue.


@issuehunt-oss issuehunt-oss bot added the 💵 Funded on Issuehunt This issue has been funded on Issuehunt label May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💵 Funded on Issuehunt This issue has been funded on Issuehunt enhancement help wanted
Projects
None yet
Development

No branches or pull requests

5 participants