Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async iterator does not work with Duplex streams #33130

Closed
szmarczak opened this issue Apr 28, 2020 · 3 comments
Closed

Async iterator does not work with Duplex streams #33130

szmarczak opened this issue Apr 28, 2020 · 3 comments

Comments

@szmarczak
Copy link
Member

  • Version: v14.0.0
  • Platform: Linux solus 5.5.11-151.current #1 SMP PREEMPT Tue Mar 24 18:06:46 UTC 2020 x86_64 GNU/Linux
  • Subsystem: stream

What steps will reproduce the bug?

const {Duplex, PassThrough} = require('stream');

const response = new PassThrough();
setTimeout(() => response.write('chunk 1'), 500);
setTimeout(() => response.write('chunk 2'), 1000);
setTimeout(() => response.write('chunk 3'), 1500);
setTimeout(() => response.end(), 2000);

class HelloWorld extends Duplex {
	constructor(response) {
		super();

		this.response = response;
		this.readMore = false;

		response.once('end', () => {
			this.push(null);
		});

		response.on('readable', () => {
			if (this.readMore) {
				this._read();
			}
		});
	}

	_read() {
		const {response} = this;

		this.readMore = true;

		if (response.readableLength) {
			this.readMore = false;
		}

		let data;
		while ((data = response.read()) !== null) {
			this.push(data);
		}
	}
}

const instance = new HelloWorld(response);
instance.end();

(async () => {
	await new Promise(resolve => setTimeout(resolve, 100));

	for await (const data of instance) {
		console.log(data.toString());
	}

	console.log('finished await');
})();

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

chunk 1
chunk 2
chunk 3
finished await

What do you see instead?

finished await

Additional information

Async iterator utilizes the end-of-stream module. The culprit is this:

(wState && wState.finished) || (rState && rState.endEmitted) ||

wState.finished is true, therefore the callback is called even though it should not.

Original Got issue: sindresorhus/got#1172

@szmarczak
Copy link
Member Author

Please note that it works as expected on Node.js v13.13.0.

@jasnell
Copy link
Member

jasnell commented Apr 28, 2020

/cc @ronag @mcollina

@ronag
Copy link
Member

ronag commented Apr 28, 2020

I'm on it.

ronag added a commit to nxtedition/node that referenced this issue Apr 28, 2020
finished would incorrectly believe that a Duplex is already
closed if either the readable or writable side has completed.

Fixes: nodejs#33130
@ronag ronag closed this as completed in d84f131 Apr 30, 2020
targos pushed a commit that referenced this issue May 4, 2020
finished would incorrectly believe that a Duplex is already
closed if either the readable or writable side has completed.

Fixes: #33130

PR-URL: #33133
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants