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

Completing all concurrent tasks if one errors. #34

Closed
philcockfield opened this issue Oct 16, 2016 · 26 comments · Fixed by #46
Closed

Completing all concurrent tasks if one errors. #34

philcockfield opened this issue Oct 16, 2016 · 26 comments · Fixed by #46

Comments

@philcockfield
Copy link

I'm running a set of tasks concurrently. At the moment, if one of the tasks fails, the whole execution is stopped.

Is it possible to have them all continue to completion, whether failed or passed?

Thanks

@SamVerschueren
Copy link
Owner

Just wondering why you want that? Shouldn't it just stop if it fails?

Not sure if this is enough but you can also handle that yourself if you want a certain task to not stop execution by just implementing the catch method of your promise, or surrounding the implementation with try-catch if it's synchronous.

@philcockfield
Copy link
Author

philcockfield commented Oct 16, 2016

Hey @SamVerschueren

if you want a certain task to not stop execution by just implementing the catch method of your promise, or surrounding the implementation with try-catch if it's synchronous.

What I'm looking for is the red cross if it errors. If I catch it in a try/catch is there any way to signal to show the red cross?

The use case is a script I've built that performs actions across a series of modules within a project, such as linting, eg:

image

In this case, they are all running concurrently - and the goal is to get to the end and see a complete set of what passed, and what errored out.

I'm thinking maybe a stopOnError flag which defaults to true (current behavior).

new Listr([], {
  concurrent: true,
  stopOnError: false, // Default: true
})

@okonet
Copy link
Contributor

okonet commented Nov 1, 2016

Same here. I'd like to allow https://github.com/okonet/lint-staged to run all linters and then output the results for all of them. Right now if one linter fails, the process will end. See lint-staged/lint-staged#86

I've played around with the context but still could not make this to work as I wish.

@SamVerschueren
Copy link
Owner

So if I understand correctly, it should be possible to prevent exitting if one task fails and just continue processing all the tasks until the end? Also show the error message under the failed task. I will look into that.

@okonet
Copy link
Contributor

okonet commented Nov 1, 2016

In case of lint-staged I have nested tasks:

  1. List of linters to run (for each glob)
  2. Set of commands to run for each glob

In my case, I want to run all linters (on globs) but stop running tasks of the second level whenever the error occurs. I think this is doable with the context though.

@okonet
Copy link
Contributor

okonet commented Nov 1, 2016

Also show the error message under the failed task

This is related to #16 I think

@philcockfield
Copy link
Author

@SamVerschueren - yes to your last comment. That's what I was thinking. @okonet takes it to a more sophisticated level that what I was doing ("turtles all the way down"). Smart.

@SamVerschueren
Copy link
Owner

Just to make sure before I start implementing this, I think the following implementation should cover all the use cases.

Base case

Just a regular list, no nested levels.

const list = new Listr([
    {
        title: 'foo',
        task: () => Promise.reject(new Error('bar'))
    },
    {
        title: 'unicorn',
        task: () => Promise.reject(new Error('rainbow'))
    }
], {
    exitOnError: false
});

This would result in

✖ foo
  → bar
✖ unicorn
  → rainbow

Nested level

exitOnError boils down to all the subtasks as well.

const list = new Listr([
    {
        title: 'foo',
        task: () => Promise.reject(new Error('bar'))
    },
    {
        title: 'unicorn',
        task: () => new Listr([
            {
                title: 'rainbow',
                task: () => Promise.reject(new Error('unicorn rainbow'))
            },
            {
                title: 'fiz',
                task: () => Promise.resolve('biz')
            }
        ])
    }
], {
    exitOnError: false
});
✖ foo
  → bar
✖ unicorn
  ✖ rainbow
    → unicorn rainbow
  ✔ fiz

If however, you only want this on the first level, you would define it like this.

const list = new Listr([
    {
        title: 'foo',
        task: () => Promise.reject(new Error('bar'))
    },
    {
        title: 'unicorn',
        task: () => new Listr([
            {
                title: 'rainbow',
                task: () => Promise.reject(new Error('unicorn rainbow'))
            },
            {
                title: 'fiz',
                task: () => Promise.resolve('biz')
            }
        ], {
            exitOnError: true
        })
    }
], {
    exitOnError: false
});

Also the other way around, if you only want this on the second level, you would configure it like this.

const list = new Listr([
    {
        title: 'foo',
        task: () => Promise.reject(new Error('bar'))
    },
    {
        title: 'unicorn',
        task: () => new Listr([
            {
                title: 'rainbow',
                task: () => Promise.reject(new Error('unicorn rainbow'))
            },
            {
                title: 'fiz',
                task: () => Promise.resolve('biz')
            }
        ], {
            exitOnError: false
        })
    }
]);

Do I miss something? Is this approach flexible enough to cover all the use cases?

@okonet
Copy link
Contributor

okonet commented Nov 1, 2016

This is looking good to me. Awesome!

@philcockfield
Copy link
Author

👍 from me too. Looks great @SamVerschueren!

@SamVerschueren
Copy link
Owner

I will have a look at it, probably this week. Thanks for the feedback! If anything pops up in your mind, don't hesitate to add a comment :).

@frederickfogerty
Copy link

Hey 😄 just wondering if there was a branch or anything I could test this out on - this would be very useful for us.

@SamVerschueren
Copy link
Owner

I've started working on this so I hope to land this soon.

@frederickfogerty
Copy link

Awesome @SamVerschueren! Thank you so much 🙌🏻

@philcockfield
Copy link
Author

philcockfield commented Jan 24, 2017

Magnificent @SamVerschueren!! 🥇
Thanks

@SamVerschueren
Copy link
Owner

Alright, I have an update. I have this working, but it's not working in the following use case. It might be an edge case though but wanted to see what you guys think.

const tasks = new Listr([
	{
		title: 'Foo',
		task: () => Promise.reject(new Error('Foo bar'))
	},
	{
		title: 'Bar',
		task: () => {
			return new Listr([
				{
					title: 'Foo Bar Subtask 1',
					task: () => Promise.reject(new Error('Foo bar baz'))
				},
				{
					title: 'Foo Bar Subtask 2',
					task: () => delay(2000)
				}
			], {concurrent: true, exitOnError: true});
		}
	}
], {
	exitOnError: false
});

So at the first level, exitOnError is set to false which means the first task fails (because it rejects) but the second task will just start as expected. The second task returns a subtask which has concurrent set to true AND exitOnError set to true. Because of the tasks being executed concurrently, there is no way to stop Foo Bar Subtask 2 from running

So this seems to play nice with serial tasks, but it will cause issues when you start to combine them with concurrent tasks because I don't think there is a way I can halt the execution from the other tasks in the concurrent list.

@frederickfogerty
Copy link

@SamVerschueren I don't see the problem (maybe I don't understand). Isn't this how concurrent works currently? i.e. Foo Bar Subtask 2 runs anyway, whether Foo Bar Subtask 1 succeeds or fails.

To me, this is acceptable behaviour. The thing I'm interested in from the exitOnError is the completion of the tasks, not the starting of tasks. If I wanted to control the starts, I wouldn't make the tasks concurrent, which would allow me to use skip et al. to control the starting of the tasks.

@SamVerschueren
Copy link
Owner

I discussed this with @sindresorhus and we came up with a solution. Promises aren't cancellable, observables are. Because Listr supports observables, cancellation for those should work (to be tested but let's assume it works).

We could support an onCancel hook and work with p-cancelable in the background. The cancellation of the promise should be handled by the developer itself.

const tasks = new Listr([
	{
		title: 'Foo',
		task: () => Promise.reject(new Error('Foo bar'))
	},
	{
		title: 'Bar',
		task: () => {
			return new Listr([
				{
					title: 'Foo Bar Subtask 1',
					task: () => Promise.reject(new Error('Foo bar baz'))
				},
				{
					title: 'npm install',
					task: (ctx, task) => {
						const cp = execa('npm', ['install']);

						task.onCancel(() => {
							cp.kill();
						});

						return cp;
					}
				}
			], {exitOnError: true});
		}
	}
], {
	exitOnError: false
});

Feedback more then welcome!

@frederickfogerty
Copy link

frederickfogerty commented Jan 25, 2017

The tasks don't cancel in the current versions of Listr, do they @SamVerschueren? This seems like a separate issue, and is not related to exitOnError

@SamVerschueren
Copy link
Owner

@frederickfogerty Yes they do cancel because when one task throws, either in a subtask or whatever, it boils all the way up to the root and the process exits.

I agree, it is an edge case, but it's something I like to think through first. Based on the feedback I can still push what I have now (without cancellation) and create a separate issue and see if people need it and implement it later.

@frederickfogerty
Copy link

Oh! Of course, the whole process exits - that's the part I was missing.

Personally, I'm happy with pushing what's there now, since it's not actually breaking any existing functionality. If people want to use exitOnError with cancelling, they can use Observables for now, and then the cancellable Promises when (or if) that becomes available.

@okonet
Copy link
Contributor

okonet commented Jan 26, 2017

I agree on pushing what you have now and create a separate issue.

@SamVerschueren
Copy link
Owner

The first part of this feature landed in 0.10.0 🎉. If still anything isn't working as expected, let me know!

@frederickfogerty
Copy link

Thanks you so much @SamVerschueren! 🙌 🎉 💪

@okonet
Copy link
Contributor

okonet commented Jan 29, 2017

@SamVerschueren I've just tried to integrate it and can't wrap my head on how do I display all errors when all tasks complete. When using default renderer, it just continues silently now, so I was expecting the error argumemt in the catch to contain all my errors. Looking at the code it is not the case. Is there a way of accessing it somehow after the execution? Should it be some internal state I have to take care of? Please point me in the right direction.

@SamVerschueren
Copy link
Owner

@okonet Good point, let's discuss this in #48.

SamVerschueren pushed a commit that referenced this issue May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants