`uploadProgress` gets fired after cancelation

[Rychu-Pawel]

Describe the bug

• Node.js version: 16
• OS & version: Windows 10

Actual behavior

There is always one last progress (at least uploadProgress) event fired after the promise was canceled.

Expected behavior

After the promise is canceled and the promise rejects with ERR_CANCELED there shouldn't be any more progress events fired.

Code to reproduce

const gotChunkInstance = this.gotInstance.extend({
    retry: {
        limit: retryLimit,
        methods: ['GET', 'POST', 'PUT', 'PATCH', 'HEAD', 'DELETE', 'OPTIONS', 'TRACE']
    }
});

const ongoingRequestPromise = gotChunkInstance.post(uploadUrl, {
    body: await chunkStreamProvider.getChunkStream() //this is an FS ReadStream
});

ongoingRequestPromise.on("uploadProgress", (progress: Progress) => {
    console.log("Except normal progress logging this is also always logged one time after the ongoingRequestPromise rejects with ERR_CANCELED");
});

setTimeout(() => ongoingRequestPromise.cancel(), 2000);

await ongoingRequestPromise;

Checklist

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

[szmarczak]

Can you add console.log before this line https://github.com/sindresorhus/got/blob/d4c2913ec25968e8d99a4a24e589d3e741072369/source/core/index.ts#L1161 and see if it also gets called after cancelation? Try the same before https://github.com/sindresorhus/got/blob/d4c2913ec25968e8d99a4a24e589d3e741072369/source/core/index.ts#L1154

[Rychu-Pawel]

Added console.logs there and the conclusions are:

• this._request.write is not called after cancelation
• the (error?: Error | null) => { ... } callback is called once after the cancelation

[Rychu-Pawel]

BTW. There is no way to remove uploadProgress event listeners, right?

[szmarczak]

Added console.logs there and the conclusions are

I think you've just found an edge case. This is what's happening under the hood:

1. socket.write(data, encoding, callback);
2. socket.destroy();
3. callback gets called

This is valid (and expected) Node.js socket behavior. Probably we need to replace

https://github.com/sindresorhus/got/blob/461b3d41790c7f091ebbb9acd87d25a22e0ffb1a/source/core/index.ts#L468

with

 if (this._request.destroyed) { 

Can you confirm if this works?

BTW. There is no way to remove uploadProgress event listeners, right?

No, not yet. Please open a separate issue about this. Out of curiosity, what's your use case?

[Rychu-Pawel]

No, unfortunately, that doesn't help. I added a console.log at the very beginning of this callback and it is not called anywhere close. I mean it doesn't seem to be called anywhere during or after the cancelation nor right before or right after the last progress event is emitted.

Out of curiosity, what's your use case?

I'm building a library that will be a part of a Node CLI and an Electron desktop app that will be used by our customers to upload files to our servers. It should support file chunking and its parallel upload with the possibility to pause and resume the transfer. Retries of course too. It should also allow immediate aborting of the whole transfer (with some progress loss) but still with the possibility to resume it in some close undefined future. After long research of available options the GOT library was the one satisfying most of our needs. Alpha version of the CLI tool has already been published but it requires to create a free account on https://www.filemail.com/:

npm install -g filemail-cli
filemail upload testFile.txt -u [email protected]

[szmarczak]

Well that's quite a comprehensive description 😅 I meant what was the use case of uploadProgress to be exact, and why emitting it after cancellation doesn't work in your case. I guess the answer is tracking file uploads. That makes sense, but I still don't know why that additional uploadProgress would break the app. Anyway that's a bug and will be fixed. Thanks for reporting!

[Rychu-Pawel]

Heh. I was in a hurry to catch the weekend and didn't get your question right :) I divide the files into chunks and upload a few in parallel. Every upload progress event is used to update the progress bar. When a user aborts the whole operation I cancel ongoing promises. When the ERR_CANCELED is thrown I "rewind" the global progress by the uploaded bytes of the canceled chunks. When additional events occur for the canceled promises it unnecessarily adds some progress. When the transfer is resumed later and gets completed progress bar ends with 100.8% for example.

I added a workaround exactly as in this code snippet here https://github.com/sindresorhus/got/issues/2087#issuecomment-1192759438 with an additional comment in the code but still would be great not to have this extra event after the promise was canceled :)

[Rychu-Pawel]

I tried to reproduce this with a test but the below code fires the event only once so it behaves correctly. I guess it is because there are only two events in total in this unit test or there are some mocks or other mechanisms in place? Posting the code maybe it will be useful for you to do some modifications to reproduce the issue:

test('upload progress - should not fire events after cancelation', withServer, async (t, server, got) => {
	server.post('/', uploadEndpoint);

	const events: Progress[] = [];

	const ongoingRequestPromise = got.post({body: file});

	let hasCanceled = false;

	void ongoingRequestPromise.on('uploadProgress', (event: Progress) => {
		console.log("event");
		if (hasCanceled) {
			events.push(event);
			console.log("event pushed");
		} else {
			console.log("before cancel");
			ongoingRequestPromise.cancel();
			console.log("after cancel");
		}
	});

	try {
		await ongoingRequestPromise;
	} catch (error) {
		if (error instanceof RequestError && error.code === 'ERR_CANCELED') {
			hasCanceled = true;
			console.log("hasCanceled = true");
		}
	}

	t.true(hasCanceled);
	t.is(events.length, 0);
});

[szmarczak]

Ah, the progress bar, you're right! I'll fix this soon.

[szmarczak]

This should be fixed now in [email protected]. Please confirm.

[Rychu-Pawel]

Sorry for this late response. I removed a workaround from my code and tried to reproduce this issue with no luck so looks like it has been successfully fixed 🎊