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]

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%)	⬆️
src/electron.js	47.42% <80%> (+47.42%)	⬆️

[melanieseltzer]

Unfortunately this is still not killing the servers properly for me on Mac. I'm testing with both X button and "Quit Guppy" in the menu bar but it's not killing the server on 3001.

I dug into it and maybe found the reason? Not too sure, but this discrepancy is interesting.

When we are toggling on the dev server we are sending along the PID to the main process via the following (in task.saga.js):

yield call([ipcRenderer, ipcRenderer.send], 'addProcessId', child.pid);

I console logged this PID when we receive it in electron.js like so (plus the processIds array):

ipcMainHandle.on('addProcessId', (event, processId) => {
    processIds.push(processId);
    notify('addProcessId', processIds);
    console.log('PID from dev server: ', processId);
    console.log('processIds array: ', processIds);
  });

As you can see in the following screenshot, the PID logged doesn't match the PID of the actual server that's open on 3001. Not really sure where it's getting this different PID.

[melanieseltzer]

Is there a reason for this? Specifying it as false kills the reporting for each individual test if run via filename regex pattern. I find it pretty helpful to see that output if I'm only running tests for a single file e.g:

[AWolf81]

Oh OK, I used it to enable console.log for debugging. I haven't noticed that this behaviour is changed. I'll remove it.

[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]

Very nice, clean tests, just a few small points of feedback. I'm going to investigate further on Mac and attempt to replicate some of the behavior @melanieseltzer and others have noticed, I'll report back if I find anything.

[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!

[melanieseltzer]

Curious why change from forEach to map here?

[superhawk610]

This is a strange pattern if you haven't seen it before. Promise.all(promiseArr) waits for every Promise in promiseArr to resolve before resolving itself. It should make more sense if I write it out without using .map notation:

const mapCallback = processId => {
  processIds = processIds.filter(id => id !== processId);
  return killProcessId(processId);
};

// processIds = [1, 2, 3];
await Promise.all([
  mapCallback(1),
  mapCallback(2),
  mapCallback(3),
]);

killProcessId returns a Promise, and we need to get that promise into the array that Promise.all() is using. This essentially boils down to:

await Promise.all([
  killProcessId(1),
  killProcessId(2),
  killProcessId(3),
]);

Does that help?

[melanieseltzer]

Nice, I knew it was due to needing a return value but for some reason I wasn't connecting the dots as to what exactly it was returning 😆 Clever.