Server side aborted request with Transfer-Encoding: chunked never resolve/reject

[darksabrefr]

• Node.js version: 13.9.0 at least to 13.11.0
• OS & version: Ubuntu 19.10

A server side aborted request with Transfer-Encoding: chunked header conduces got to never resolve nor reject. I think got doesn't listen aborted event from IncomingMessage. The stream version of got is affected too and stream never emit end, abort or error but stays in a stale state.

const http = require('http');
const got = require('got');

// http server that aborts a chunked request
const server = http.createServer((req, res) => {
	res
		.on('error', (e) => {
			console.log(`server error: ${e}`)
		})
		.on('close', () => {
			console.log('server close')
		})
		.on('end', () => {
			console.log('server end')
		})
		.on('finish', () => {
			console.log('server finish')
		});

	res.writeHead(200, { 'Content-Type': 'text/plain' });
	res.write('chunk 1');

	setTimeout(() => res.write('chunk 2'), 1000);
	setTimeout(() => res.destroy(), 2000);

	// this destroy version leads to the same result
	// setTimeout(() => res.socket.destroy(), 2000);

	// a non-aborting version would call res.end() as it
	// setTimeout(() => res.end(), 2000);
});
server.listen(8000);

// got request
(async () => {
	try {
		const gotOptions = {timeout: {socket: 2000, request: 3000}};
		const res = await got('http://localhost:8000', gotOptions);
		console.log(`client res: res=${JSON.stringify(res.body)}`);
	} catch (e) {
		console.log(`client error: error=${e}`);
	}
})();

This only outputs server close but no client response because the got promise never resolves nor rejects even with timeouts set. IncomingMessage doesn't emit end event in this case (at least in the latest node version, maybe this affirmation was not true in past versions, see: nodejs/node#25081, from where the examples here are derived) but an aborted event. To show it, you can do that:

const http = require('http');

// http server that aborts a chunked request
const server = http.createServer((req, res) => {
	res
		.on('error', (e) => {
			console.log(`server error: ${e}`)
		})
		.on('close', () => {
			console.log('server close')
		})
		.on('end', () => {
			console.log('server end')
		})
		.on('finish', () => {
			console.log('server finish')
		});

	res.writeHead(200, { 'Content-Type': 'text/plain' });
	res.write('chunk 1');

	setTimeout(() => res.write('chunk 2'), 1000);
	setTimeout(() => res.destroy(), 2000);

	// this destroy version leads to the same result
	// setTimeout(() => res.socket.destroy(), 2000);

	// a non-aborting version would call res.end() as it
	// setTimeout(() => res.end(), 2000);
});
server.listen(8000);

// standard http request
const req = http.get('http://localhost:8000', res => {
	console.log('Status code:', res.statusCode);
	console.log('Raw headers:', res.rawHeaders);

	res
		.on('data', (data) => {
			console.log(`client data: complete=${res.complete}, aborted=${res.aborted}, data=${data.toString()}`)
		})
		.on('error', (e) => {
			console.log(`client error: complete=${res.complete}, aborted=${res.aborted}, error=${e}`)
		})
		.on('aborted', () => {
			console.log(`client aborted: complete=${res.complete}, aborted=${res.aborted}`)
		})
		.on('close', () => {
			console.log(`client close: complete=${res.complete}, aborted=${res.aborted}`)
		})
		.on('end', () => {
			console.log(`client end: complete=${res.complete}, aborted=${res.aborted}`)
		});
});

req.on('error', (e) => {
	console.log(`client error: error=${e}`);
});

Here, you can see a more precise output:

Status code: 200
Raw headers: [
 ... some headers ...
  'Transfer-Encoding',
  'chunked'
]
client data: complete=false, aborted=false, data=chunk 1
client data: complete=false, aborted=false, data=chunk 2
client aborted: complete=false, aborted=true
client close: complete=false, aborted=true
server close

Note there's no client end event but an aborted one. I have also included two statuses properties from IncomingMessage: complete (true after an end) and aborted (true after an aborted event)

Checklist

• I have read the documentation.
• I have tried my code with the latest version of Node.js and Got.

[darksabrefr]

This problem still occurs with node 13.11.0, so I tried to investigate this. It seems the issue is caused by this catch:

got/source/get-response.ts

Lines 29 to 36 in 62b3388

	return pipeline(
	response,
	progressStream
	).catch(error => {
	if (error.code !== 'ERR_STREAM_PREMATURE_CLOSE') {
	throw error;
	}
	});

This change relates to #990.
If the catch is deleted, then the premature close is thrown but... only with node v13.11.0+ ! I've tried this with node v13.9.0 (without the auto destroy on pipelines, introduced in node v13.10.0) and no error is thrown.
To be compatible with node before v13.11.0, you need to add this before line 29:

response.once('aborted', () => {
    progressStream.destroy();
});

[szmarczak]

Can you try the latest v12 Node.js version?

[darksabrefr]

Can you try the latest v12 Node.js version?

Sure. Result with node v12.16.1 is different as error thrown by the pipeline has an undefined code, and so, the catch can throw the error. The got promise then rejects but with a different error of the one from node v13 branch (where e.code === 'ERR_STREAM_PREMATURE_CLOSE').:

[darksabrefr]

I've tested with an older v12 release too (v12.9.0) and the result is different comparing to v13 branch or latest v12. With the base got code, the got promise resolves with data sent before abortion. So we have a potentially truncated result but we don't have any information of this.
But if I add the aborted listener on response, the result is the same as the v12.16.1 one: the got promise rejects with the same error: an error with no code defined but a message saying ReadError: premature close

[pqvst]

Fwiw, I just stumbled upon this issue and I can reproduce it in both v13.6.0 and v13.12.0.

[szmarczak]

Tested on master (merged #1051 so many bugs got fixed) and it gives me:

server close
server close
server close
client error: error=TimeoutError: Timeout awaiting 'request' for 3000ms

Please let me know if it's fixed on your side too or not.

[pqvst]

I just tried the v11 beta. Seems to be working for me now 👍

[darksabrefr]

It should not be handled by timeouts and throws a timeout error, even if promises now resolve/reject or streams emit error event with got v11 (that is a good thing) because it's not a timeout. If timeouts are set at an upper value, say a minute as an example, the error will not be caught and transmitted until.
There is a true event when this occurs: the aborted one (as node doc says: https://nodejs.org/api/http.html#http_event_aborted). There is no error or end event emitted by node's http module (there's a close one, but it possibly depends on emitClose streams' param). The fact there is a specific event for that and not a simple true error is out of our scope, and maybe be submitted to node's team.

In got v11 it should be there:

got/source/core/index.ts

Lines 962 to 969 in bf0b0cd

	response.once('end', () => {
	this[kResponseSize] = this[kDownloadedSize];
	this.emit('downloadProgress', this.downloadProgress);
	});
	response.on('error', (error: Error) => {
	this._beforeError(new ReadError(error, options, response as Response));
	});

You have to add an additional listener on this like:

response.on('aborted', () => {
	// there is no error with this event, so create one
	// (it should be improved to have a specific error with an error code, etc...)
	const error = new Error('Request abort');
	this._beforeError(new ReadError(error, options, response as Response));
});

With that, the output is:

client error: error=ReadError: Request abort
server close

which is better, doesn't depends on timeouts settings and is handleable by applications.

[darksabrefr]

Maybe these node's github issues can help to understand this problem:
nodejs/node#28172
nodejs/node#31630

[darksabrefr]

Another node's github issue, they tried to handle this correctly but failed: nodejs/node#28677

[szmarczak]

@rarkins #1062 (comment) seems to be related with this I think

[darksabrefr]

I don't know if this is the same problem as there is no ECONNRESET error here. The problem is really between got and node's http lib. Node's IncomingMessage can emit three different events to say no more data will be emitted: error, end or aborted (and a close after each, in theory).

Got v11 listen only two of these, error and end, so the aborted cases just hang programs.

Got v10 used stream.pipeline where some node's workaround seems to have been done to transform IncomingMessage's aborted events in error ones. Unfortunately, got filtered out these errors to never throw anything, so it hangs programs too with node v13. With node v12, got fails to filter the error, so it is thrown or got promise resolves with a truncate response, depending on node's v12 minor version.

I can't remind how it worked internally in got v9, but I pretty sure this version was not hanging on aborted events, because I wrote some code to detect and handle truncated responses in this epoch.

[szmarczak]

I don't know if this is the same problem as there is no ECONNRESET error here.

Timeouts can cause ECONNRESET.

[szmarczak]

Got v11 listen only two of these, error and end, so the aborted cases just hang programs.

not anymore (wait for a release)

[szmarczak]

Got v10 used stream.pipeline where some node's workaround seems to have been done to transform IncomingMessage's aborted events in error ones.

There has been no workaround like this. You're confused.

[szmarczak]

I can't remind how it worked internally in got v9

It still had that bug. Since Got v8 (when I joined Got).

[szmarczak]

Got v10 used stream.pipeline where some node's workaround seems to have been done to transform IncomingMessage's aborted events in error ones.

I think you're talking about the premature close errors but I'm not sure... There are plenty of reasons why premature close could be thrown. It's pure speculation.

[darksabrefr]

Timeouts can cause ECONNRESET.

ECONNRESET occurs when the underlying socket is destroyed before the request finish.
The socket is closed right after aborted occurs (close event, https://nodejs.org/api/http.html#http_event_close_2), Am I right ? So it can't conduce to this error, first because aborted and error (and end) are mutually exclusive, and then because there's no more attached socket to emit this one. I can say too I have let running programs for more than 12 hours in an hang state, and nothing happened. But maybe keep-alive changes things (however, my hanging program uses keep-alive)

There has been no workaround like this. You're confused.

I think you're talking about the premature close errors but I'm not sure... There are plenty of reasons why premature close could be thrown. It's pure speculation.

Yes it's pure speculation, but I'm pretty sure node's pipelines handle this problem. Maybe not directly by listening the aborted event but via the close one, that is emitted in all cases after all other. This can lead to throw the premature close error when no end or error events are emitted before the close one. Or a thing like that, but it's more logical regarding the error name.

It still had that bug. Since Got v8 (when I joined Got).

I'm really sure too for got v9: there's no hang related to this issue, my code and tests have been written with got v9. I have done some code to workaround this problem when I migrated to got v10. The major difference is got v9 resolves the promise or ends the stream without error and with a truncated response where got v10 simply hang.

But It's possible too there's node's updates caused behaviour changes, as they done quasi-breaking changes on these modules the last weeks...

[szmarczak]

got v10 simply hang

Then I'm almost sure it was caused by the premature close errors, which just were ignored.

I'm really sure too for got v9: there's no hang related to this issue, my code and tests have been written with got v9.

The bug was still present, it just was failing silently:

https://runkit.com/szmarczak/5e9858d5a67271001a07e593

[szmarczak]

ECONNRESET occurs when the underlying socket is destroyed before the request finish.

It occurs when the socket is closed without emitting a response event.

[darksabrefr]

I can confirm with the brand new got v11, an error is thrown/emitted on server abort after response cases in http 1.x context. Good job @szmarczak and thank you!
I need to test http 2 now!

[szmarczak]

If you see any bug, make an issue stright away! I've fixed two already and I'm going to release v11.0.1 in a few mins...

[dpickett]

fwiw I landed on this issue when I encountered a resolved, chunked, response - it took some time to isolate that the transfer-encoding was the culprit because I just received a blank body.

This was on Node 14.0. When I switched to Node 12.17 given the issues noted above it works as I would have expected. The promise resolves with the fully downloaded response. I upgraded from 14.0 to the 14.5 minor and re-ran my little test and it works as expected. lol node.

I anticipate others may experience similar confusion on Node for Node 14 <= 14.5. Hoping I can save someone an hour or two trying to track that one down if they happen to stumble across this issue.

[darksabrefr]

fwiw I landed on this issue when I encountered a resolved, chunked, response - it took some time to isolate that the transfer-encoding was the culprit because I just received a blank body.

This was on Node 14.0. When I switched to Node 12.17 given the issues noted above it works as I would have expected. The promise resolves with the fully downloaded response. I upgraded from 14.0 to the 14.5 minor and re-ran my little test and it works as expected. lol node.

I anticipate others may experience similar confusion on Node for Node 14 <= 14.5. Hoping I can save someone an hour or two trying to track that one down if they happen to stumble across this issue.

In fact, I think your problem is related to this issue: #1295 which is a node 14 fixed issue.

[dpickett]

agreed. Thank you @darksabrefr