forked from yarnpkg/yarn
-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[BUGFIX yarnpkg#7924] Fix Race in
yarn pack stream code
In some versions of Node.js (such as 12.16.2), yarn pack no-longer was running the `postpack` hook. 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. In-fact the process was appearing to exit prematurely.
```js
// 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:
1. a Node.js process exits once it's event queue has been drained.
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 solve!
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:
```js
stream.pipe(otherStream);
`"
Under most circumstances it is likely wise to use
```js
const { pipeline } = require(‘stream’);
pipeline(stream, otherStream, err => {
// node-back
});
`"
And if you are using streams with promises, consider first promisifying `pipeline`
```js
const { promisify } = require(‘util’);
const pipeline = promisify(require(‘stream’).pipeline)
`"- Loading branch information
1 parent
e5327c5
commit 6b02ee6
Showing
3 changed files
with
126 additions
and
6 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters