Stream finished does not always work with http incoming message

[misos1]

• Version: v15.12.0
• Platform: Darwin Kernel Version 20.3.0
• Subsystem: http

What steps will reproduce the bug?

The stream.finished never resolves or rejects when applied onto a destroyed incoming message like in example below. It finishes for example when applied on a destroyed file stream. Also it finishes when the line with await new Promise(r => setTimeout(r, 1000)); is commented. This looks really inconsistent.

let http = require("http");
let { finished } = require("stream/promises");

let server = http.createServer(async function(req, res)
{
	for await (let chunk of req) break;
	await new Promise(r => setTimeout(r, 1000));
	console.log("waiting");
	await finished(req);
	console.log("sending");
	res.end();
});

(async function()
{
	await new Promise(resolve => server.listen(resolve));
	let req = http.request({ port: server.address().port, method: "post" }).end("abc");
	try
	{
		let res = await new Promise((resolve, reject) => req.on("response", resolve).on("error", reject));
		await finished(res.resume());
	}
	catch(e)
	{
		console.log(e);
	}
}());

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

Always.

What is the expected behavior?

Error: socket hang up
waiting
sending

What do you see instead?

Error: socket hang up
waiting

Additional information

[Linkgoron]

This works correctly for me on Node 16.0+, but not on Node 15.0+.

[Linkgoron]

Changing finished to the unpromisified version, and just calling destroy on the request, this doesn't work correctly on Node 12.22.1 and 14.17 as well.

let http = require("http");
let { finished } = require("stream");

let server = http.createServer(async function(req, res)
{
	req.destroy()
	console.log("waiting");
        await new Promise(res => setTimeout(res,1));
	await new Promise((res) => finished(req, res));
	console.log("sending");
	res.end();
});

(async function()
{
	await new Promise(resolve => server.listen(resolve));
	let req = http.request({ port: server.address().port, method: "post" }).end("abc");
	try
	{
		let res = await new Promise((resolve, reject) => req.on("response", resolve).on("error", reject));
		await finished(res.resume());
	}
	catch(e)
	{
		console.log(e);
	}
}());

[misos1]

Seems on nodejs 16 it is working correctly.

[Linkgoron]

Going over the code, I think that this was fixed by #33035 as the above works correctly in 15.5 and doesn't work in 15.4 and 15.6 (as #33035 was reverted).

[mcollina]

cc @nodejs/http @nodejs/streams

[mcollina]

FYI: Node v15 is going out of maintenance in a bit more than a month, it is unlikely to receive a fix for this.

A few notes:

1. do not use async/await in a event handler or a callback. This will lead to unintended results and broken behaviors.
2. don't write this code. If you call req.destroy(), you need to assume that the response has been destroyed as well: stop all operations for that request.
3. we do have bug somewhere, I'm not sure the fix is backportable.

@dnlup @ronag do you think it could be possible to fix this in v12 and v14 in a non-breaking way?

[spazmodius]

do not use async/await in a event handler or a callback

Wait, what? Can you point to some elaboration on this, please?

[benjamingr]

Wait, what? Can you point to some elaboration on this, please?

Instead of doing:

emitter.once('foo', async () => {
  // no one is awaiting this, some stuff has best effort cleanup
  await someOtherStuffOrCleanup();
});

vs.

await once(emitter, 'foo');
await someOtherStuffOrCleanup(); // now everything is listened for and is in sequence.

[spazmodius]

@benjamingr I understand your point about orphaning a Promise, but I'm missing how that relates to the examples leading up to @mcollina 's comment.

I surmise that the "event handler or callback" he was referring to is the createServer() callback. To "not use async/await" in that callback I can only interpret in these possible ways, neither of which is tenable:

1. The callback must call res.end() before it returns.
2. res.end() must be buried in more callbacks, not after await.

What am I not getting?

[ronag]

If you throw inside the async function it will become an unhandled exception. await finished can throw. Even it if might be fine in certain callbacks, it sometimes can cause weird ordering issues in edge cases and it's difficult to determine when this is a problem or not. It's better as a general rule to avoid async/await in callbacks.

[misos1]

I could create an async generator from createServer. Btw is there a plan to have a promise/generator based http?

[spazmodius]

@ronag

If you throw inside the async function it will become an unhandled exception

I disagree. Throwing in an async function is obviously catchable.

Maybe you mean "throwing in an event handler will be unhandled"? But then, that is true whether it is sync or async.

[mcollina]

I mean literally: do not mix promises and callbacks. This is a good talk on the topic from @jasnell: https://youtu.be/XV-u_Ow47s0.

I would recommend you to use Koa, Hapi or Fastify (and others), as they provide a higher level API that is promise-friendly. There are a few tricky bits in using the low-level APIs with async await that you do not want to deal with.

---

The code you are proving is something that I would not recommend anybody to write. You are deliberately destroying something and then checking for it to be destroyed. Note that when you destroy the request you also destroy the response - there is no reason to continue.