[Bug] ZipFS ReadStream missing 'end' event sometimes on Node v14

[lefb766]

• I'd be willing to implement a fix

Describe the bug

A stream object returned from fs.createReadStream() is supposed to emit end event, but when reading a certain file from ZipFS, the stream emits 'close' but no 'end'.

To Reproduce

A Sharlock repro is given, but it's not perfect and can be flaky. Because we need Node.js v14.1.0 or higher to reproduce the problem properly.

Apparently, the Sharlock repro doesn't runs on a newer Node and we have 'end' event emitted but in different order leading the test to fail. I'm not sure if the order is a problem. If not, the Sharlock test is flaky. However, from my observation, it is likely that only the files which get the events in reverse order on older Node miss 'end' on v14.1.0+.

I made a more proper repro on my repo.

git clone https://github.com/lefb766/yarn-zipfs-bug-repro
cd yarn-zipfs-bug-repro
yarn install
yarn exec node index.js

This code interacts with ZipFS directly and prints file names in @yarnpkg/fslib zip file from .yarn/cache along with emitted event names from its ReadStream.
On Node.js v14.1.0, the program stops in the middle of execution (see nodejs/promises-debugging#16).

Here's Sharlock repro.

Reproduction

---

Playground

await packageJsonAndInstall({
dependencies: {
[`@yarnpkg/fslib`]: `2.3.0`,
},
});

const CODE = `
const fs = require('fs');

const filePath = require.resolve('@yarnpkg/fslib/lib/ZipFS.js');

const stream = fs.createReadStream(filePath);
const nullStream = fs.createWriteStream('/dev/null');

stream.on('end', () => console.log('end'));
stream.on('close', () => console.log('close'));
stream.on('error', () => console.log('error'));

stream.pipe(nullStream);
`;

require('fs').writeFileSync('./index.js', CODE);

await expect(yarn('node', './index.js')).resolves.toBe('end
close
');

Output:

Error: �[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mresolves�[2m.�[22mtoBe�[2m(�[22m�[32mexpected�[39m�[2m) // Object.is equality�[22m

�[32m- Expected�[39m
�[31m+ Received�[39m

�[32m- �[7mend�[27m�[39m
�[32m- close�[39m
�[31m+ close�[39m
�[31m+ �[7mend�[27m�[39m
�[2m  ↵�[22m
at Object.args [as toBe] (/sandbox/node_modules/expect/build/index.js:202:20)
at module.exports (evalmachine.<anonymous>:25:51)
at process._tickCallback (internal/process/next_tick.js:68:7)

---

Screenshots

If applicable, add screenshots to help explain your problem.

Environment if relevant (please complete the following information):

• OS: Linux, Windows
• Node version v14.14.0
• Yarn version 2.3.3

Additional context

It can be an upstream problem, but I'm suspecting the problem can root to how ZipFS ReadStream is implemented.

berry/packages/yarnpkg-fslib/sources/ZipFS.ts

Lines 451 to 463 in 7929629

	const stream = Object.assign(new PassThrough(), {
	bytesRead: 0,
	path: p,
	close: () => {
	clearImmediate(immediate);
	closeStream();
	},
	_destroy: (error: Error | undefined, callback: (error?: Error) => void) => {
	clearImmediate(immediate);
	closeStream();
	callback(error);
	},
	});

[yarnbot]

Hi! 👋

This issue looks stale, and doesn't feature the reproducible label - which implies that you didn't provide a working reproduction using Sherlock. As a result, it'll be closed in a few days unless a maintainer explicitly vouches for it or you edit your first post to include a formal reproduction (you can use the playground for that).

Note that we require Sherlock reproductions for long-lived issues (rather than standalone git repositories or similar) because we're a small team. Sherlock gives us the ability to check which bugs are still affecting the master branch at any given point, and decreases the amount of code we need to run on our own machines (thus leading to faster bug resolutions). It helps us help you! 😃

If you absolutely cannot reproduce a bug on Sherlock (for example because it's a Windows-only issue), a maintainer will have to manually add the upholded label. Thanks for helping us triaging our repository! 🌟