[@rushstack/package-deps-hash] Wait for stream close when async executing git commands

[akres]

Fixing the race condition mentioned in microsoft/rushstack-websites#231

Summary

As mentioned in the issue, this code contains a race condition risk. The function sets up data event handlers for stdout and stderr streams. Then it waits for the exit event on the process. However, it is possible for the streams' data and close events to arrive after the exit event. In that case, the function returns an empty string, instead of the actual output.

Details

The fix is that we need to wait for the streams to close to get the full output. The node docs state that "The 'close' event is emitted after a process has ended and the stdio streams of a child process have been closed.", but when I tried changin the awaited event from exit to close, it didn't help. So the solution is to wait for all three events.

They also need to be awaited in parallel, because if we first wait for the close event, but the exit event comes first, the following wait for exit hangs the process entirely, because the exit event had already come before we started waiting for it.

How it was tested

The manifestation of this issue is randomly misbehaving incremental build when there are any uncommited changes in the repo.
Suppose I run rush build 5x in a row:
1st run builds everything
2nd run skips everything
3rd run builds packages that contain uncommited changes, even though there was no change done to the files between runs 2 and 3
4th run does the same asi 3rd
5th run skips everything

The race condition occurs on the 3rd run. It causes the git status call to return an empty string, so the deps hash returns hashes of files as they are in HEAD. But the previous run saved the hashes of the changed files, so there is a change detected and the build is triggered. In the 4th run the git status call returns the correct data, but the 3rd saved the incorrect hashes, to the build is triggered again.

This would happen randomly, 5-20% of executions.

I've tested this by running rush build 1000x in sequence and I haven't received a single rebuild or hang, so it should be solid now.

[iclanton]

@akres - Can you sign the CLA?

[akres]

@akres - Can you sign the CLA?

Hi, @iclanton, I need to double check with my company that it is OK. I expect that I will do it tomorrow (my tomorrow, so if all goes well should be signed when you folks show up)

[akres]

@microsoft-github-policy-service agree