[BUGFIX #7924] Fix Race in yarn pack stream code

[stefanpenner]

TODO:

• root cause and understand the domain
• write better commit message
• manual test
• yarn specific tests

---

In some versions of Node.js which include nodejs/node@0e89b64 (such as 12.16.2) , yarn pack no-longer runs postpack. Debugging the code in question (following example) lead us to notice that the following await never progressed, the streams error and close handlers were never invoked, and that the process was appearing to exit prematurely.

// src/cli/commands/pack.js

await new Promise((resolve, reject) => {
 stream.pipe(fs2.createWriteStream(filename));
 stream.on(‘error’, reject); // reject is never invoked
 stream.on(‘close’, resolve); //  resolve is never invoked
 // reached
});

// never reached

What is going on here?

As it turns out, the above stream code is unsafe, and only appeared to function do to a specific implementation detail of zlib.Gzip. Once that implementation detail changed in Node.js; the process would exit while awaiting the above promise, and thus would leave the code which triggers the postpack hook unreachable.

The facts (As we know them so far):

1. a Node.js process exits once it's event queue has been drained and has no outstanding reference-able handles (such as IO, Timers etc).
2. stream.pipe(…) does not add a task to the event queue
3. new Promise (…) does not add a task to the event queue
4. prior to node 2.16, an implementation of zlib.Gzip added a task to the event queue
5. nodejs/node@0e89b64 changed that behavior (confirmed via bisect)
6. in node 2.16 (and several other versions) yarn pack would exit prior to invoking the postpack hook.

Mystery solved!

That's a lot going on, so how can one safely use streams ?

Luckily Node.js has a new utility method (node >= 10) const { pipeline } = require(‘stream’); This higher level utility has less caveats than stream.pipe / stream.on(…) and appears to be preferred in Node.js's own documentation has been re-worked to use pipeline for all but the most specific low level operations.

In short, rather then:

stream.pipe(otherStream);

Under most circumstances it is likely wise to use

const { pipeline } = require(‘stream’);

pipeline(stream, otherStream, err => {
  // node-back
});

And if you are using streams with promises, consider first promisifying pipeline

const { promisify } = require(‘util’);
const pipeline = promisify(require(‘stream’).pipeline)

---

@krisselden provided validation of the issue, updated my mental model of streams and ultimately the appropriate most resilient solution.