Use Node's native spawnSync for execSync to improve performance

[synaptiko]

Hello,

I tried to rewrite execSync method with usage of Node's native spawnSync as it should improve performance of sync version of shell.exec method rapidly.

According to my own test script the improvement is from original 12s to 1.6s after change.

What do you think?

I had to remove one test and I hope it works the same way as before. I don't have a Windows machine so it would be great to confirm it works the same also under Windows.

Btw. it could also fix following issues:

• phpcs very slow #440, Massive CPU usage on exec() windows #108

[codecov-io]

Codecov Report

Merging #758 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #758      +/-   ##
==========================================
- Coverage   97.38%   97.33%   -0.06%     
==========================================
  Files          33       33              
  Lines        1222     1199      -23     
==========================================
- Hits         1190     1167      -23     
  Misses         32       32

Impacted Files	Coverage Δ
src/exec.js	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38b57c8...5a4716b. Read the comment docs.

[freitagbr]

Here's the problem: with stdio: 'inherit', c.stdout and c.stderr are both null. However, we need to be able to return stdout and stderr from the call to exec, as that is part of the API.

[synaptiko]

Thanks, you're right. I didn't realize this. It's fixed now.

Btw. I noticed strange behavior. If you run an interactive command with exec, eg. git commit (with vim configured as an EDITOR) previous version of execSync will end up in not so functional state, you have to basically end up the script with Ctrl+C.

The current version just print out something like this:

Vim: Warning: Output is not to a terminal
Vim: Warning: Input is not from a terminal
Vim: Error reading input, exiting...

Vim: Finished.
error: There was a problem with the editor 'nvim'.
Please supply the message using either -m or -F option.

[nfischer]

I haven't reviewed this carefully, but this will almost certainly break behavior (which is why I was going to write an alternative to exec in #524).

Potential issues I can think of off the top of my head:

• Can it still execute commands like echo first command ; echo second command?
• Can it still expand globs like git add *.txt?

[synaptiko]

@nfischer I tested both cases and they seem to be working correctly. I also added test for the first case.

I checked your PR with cmd and it looks something I could use too :-) Next time I need to check other PRs first :-)

[synaptiko]

@nfischer Oh, sorry, I was too eager… The newly added test failed on Node v8 (I run node 6.11 locally)…

Hmm, ok, not sure about my "fix" then…

[synaptiko]

I overlooked that appveyor runs tests on Windows. It probably should pass even on Windows? The test wasn't here before so I'm not sure about the previous version of execSync…

[nfischer]

I tested both cases and they seem to be working correctly. I also added test for the first case.

Thanks for checking. It looks like we see different behavior for Windows and Unix for spawnSync().

Also, how does this handle output? shell.exec('echo foo ; sleep 5 ; echo bar') should write to the console in real time, not dump all output at the end. Do we have the correct behavior for this patch?

[synaptiko]

@nfischer Unfortunately real time output is not implemented. I've been playing with it for a while but I cannot figure out any solution. Surprisingly it leads to the original solution of execSync :-) At least I understand the original solution now. My solution cannot be used in case the API has to remain the same and real time output is required too.

I think now… the bottleneck of the original solution, IMO, is that for every command there is spawned node process and several files are created. What if this node process and files are created only once for the first execSync call? We could then use IPC to send the command and options. What do you think? Does it make sense?

[nfischer]

My solution cannot be used in case the API has to remain the same and real time output is required too.

exec() has several problems with it. At this point, I don't see a reason to change its behavior--I think it's best to revise completely (which is the approach I took in #524). In other words, I'd like to avoid breaking changes to exec() since we already have plans to deprecate & replace it.

What if this node process and files are created only once for the first execSync call? We could then use IPC to send the command and options. What do you think? Does it make sense?

There are some potential wins here. I think (but we should verify) that process.on('exit', ...) is a reliable signal to wait for cleanup (for both processes and files).

Process creation should happen lazily on the first exec() call.

Without lots of effort, we may be able to remove the exit code file and just use the process return status (which I think child_process.execSync() provides).

We can rearchitect things a bit like in #524. Right now we write an entire script to a temp file, but we can move this to a new source file and pass parameters in via process.argv. This reduces the amount we have to write to disk, which should speed things up.

Is there a way to do IPC without writing to the file system? That would be ideal, but I've yet to find a way.

[synaptiko]

@nfischer After quick research I found that IPC uses sockets, it can work with unix sockets, TCP, UDP etc.

But I think this idea won't work because IPC is completely async.

I think we can close this PR without merge as I don't know how to fix it. And if you say exec will be deprecated anyway when you finish #524 then it doesn't make sense to improve it.

Anyway, thanks for your support and time on this.

[synaptiko]

I won't continue on this PR => closed.