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