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

Awaitable q.push doesn't resolve when queue worker throws an error #1659

Closed
darksabrefr opened this issue Jun 6, 2019 · 6 comments
Closed

Comments

@darksabrefr
Copy link

await q.push(task) is not safe for use for the moment because the promise returned by q.push never resolve nor reject if the queue worker throws an error. I understand that await q.push(task) cannot throw an error because of the backward compatibility of this method and its optional callback parameter (the push and forget feature). A q.asyncPush(task) method that rejects its promise shouldn't be feasible ?

You can show the problem with this code (in node or browser without the require):

const async = require('async');
let pushResult;

(async () => {
	const q = async.queue(async (task) => {
		throw new Error('Bad thing');
	});

	try {
		pushResult = q.push({some: 'data'});
		await pushResult;
		console.log('Error not catched');
	} catch (e) {
		console.log('Error catched');
	}
})();

setTimeout(() => {
	console.log('exit');
	console.log(pushResult);
}, 1000);

Originally posted by @darksabrefr in #1641 (comment)

@aearly
Copy link
Collaborator

aearly commented Jun 6, 2019

I'm considering making q.push() resolve with the error object. Would that be sufficient? I really don't wan't to give unhandledRejections to the vast majority of queue users.

@darksabrefr
Copy link
Author

darksabrefr commented Jun 10, 2019

Resolving with an error may be an option but you will need to test the q.push return, it's not really a promise conventional pattern (no try/catch/finally block or .then()/.catch() methods can be used with). Direct rejections are not an option, I agree with it, it's a major modification. For me, there is 2 other possibilities:

  • a distinct q.asyncPush method that returns a promise that can reject. Maybe the easiest and the clearest way because the promise and callback APIs can be fully separated.
  • a boolean as second parameter of q.push to activate promises rejects.

The really important thing is that promises will not be forever pending, so you can select the best option you think ;-)

@aearly
Copy link
Collaborator

aearly commented Jun 13, 2019

I like the asyncPush idea.

@aearly aearly closed this as completed in 6739c08 Jun 23, 2019
@darksabrefr
Copy link
Author

Thank you very much !

@mjsalinger
Copy link

@aearly was this ever implemented?

@darksabrefr
Copy link
Author

It was, see https://caolan.github.io/async/v3/docs.html#QueueObject : pushAsync

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants