Fix taskkill issue 364 & add tests

[AWolf81]

Related Issue:

364 & #344

Summary:

• Changed to spawnSync so taskkill is executed before closing the app.
• Removed event.preventDefault from ipc callback as I think it's not available & not needed. IpcMain event got just two key - see the docs here

Killing tested on Ubuntu 18.0.4 (64bit) & Windows 10 Pro (64bit).

Test steps to verify task killing

1. Launch app with yarn start (executing the binary should do the same)
2. Launch a devServer (will create a listening port on 3001)
3. Verify that server is running with browser or console (Windows netstat -aon | find "3001" or lsof -i :3001 on Mac/Linux)
4. Close app with x-button and close on app icon
5. Repeat step 3 (verify that the server is stopped)

Discussion

• Does changing to spawnSync has an impact on performance? How can we check this? Maybe starting multiple devServers and check app close duration?
• How can we test ipcMain & ipcRenderer? I'd like to add a test but I haven't got it working yet. I'd like to check that adding / deleting of ids is working and that taskkill is called. OK, testing is working but it is probably not a perfect solution. Anyway, I think it's OK.

[codecov[bot]]

Codecov Report

Merging #367 into master will increase coverage by 1.46%. The diff coverage is 85.29%.

@@            Coverage Diff             @@
##           master     #367      +/-   ##
==========================================
+ Coverage   37.72%   39.18%   +1.46%     
==========================================
  Files         157      157              
  Lines        3494     3504      +10     
  Branches      440      441       +1     
==========================================
+ Hits         1318     1373      +55     
+ Misses       1900     1860      -40     
+ Partials      276      271       -5

Impacted Files	Coverage Δ
src/services/kill-process-id.service.js	100% <100%> (+53.84%)	:arrow_up:
src/electron.js	47.42% <80%> (+47.42%)	:arrow_up:

[AWolf81]

OK, that's weird maybe it's a process id of a child.

Can you please check the pids when taskkill is called? If it's the same we need to figure out which id it is storing - not sure why this is not working on Mac.

[AWolf81]

@melanieseltzer the following issue is interesting and you could try the following to find the cause of the wrong PID:

• Temporarily revert yarn version to check if it's yarn related
• How many processes are between two devServer starts? Is it incrementing by one or two. Not sure if this would help but it could be good to know if there is a sh and a node process. Maybe the PID is from a shell process.
• Maybe it could be related to Node.js child_process.spawn so switching to an older version to test this could help.

[superhawk610]

EDIT 2: Nevermind, the parent process ID is the pid of yarn start within the child project, so that should still work. Thinking...

EDIT 3: Looks like killProcessId never finishes executing. I imagine it has to do with the fact that we're assuming it's synchronous on the main thread but on Mac it relies on the callback from ps-tree and thus Electron kills it before it has a chance to fire.

EDIT 4: Hooray! Converting killProcessId to an async function and then awaiting its result successfully kills all remaining processes before exiting. I'll clean up the implementation and then push up the result in a few.

--

Alright, so here's some potential insight. Launch Guppy, start a new project ~/guppy-projects-dev/foo, launch the dev server:

# Electron log output

[IPC] addProcessId [ 41015 ]

# `ps -ef | grep 41015`

  UID PID   PPID
  501 41015 41005   0  9:51PM ttys001    0:00.41 /Users/aaronross/n/bin/node /Users/aaronross/code/guppy/node_modules/yarn/bin/yarn.js run start
  501 41026 41015   0  9:51PM ttys001    0:00.01 /bin/sh /var/folders/t6/nd_h_ks16j3clm3x9p81789w0000gn/T/yarn--1553737864312-0.1887481798573214/node /Users/aaronross/guppy-projects-dev/foo/node_modules/.bin/react-scripts start

Electron sees the pid 41015 added when the dev server for foo spins up, but that's the pid of the parent process (yarn run start). Looks like we might need to tweak the code that handles recognizing new process ids?

@melanieseltzer can you replicate this behavior? Use ps -ef to print the pid and parent pid to verify.

EDIT: Stopping the dev server prints the following:

[IPC] removeProcessId []

which seems to support the idea that the wrong pid is being passed around.

[superhawk610]

@melanieseltzer can you double-check that this now works for you on Mac?

@AWolf81 feel free to leave any review comments on my pushed code, I can't request your review since this is your PR.

EDIT: Forgot to update the tests, I'll get that fixed tonight unless someone else beats me to it.

EDIT 2: Done.

[melanieseltzer]

Tested with both options (app quit from menu bar and X button) and processes are successfully getting killed for me 🎉

Nice one, @superhawk610!