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

Fix false positive reporting of sync tasks on errors #204

Merged
merged 1 commit into from
May 20, 2020

Conversation

stof
Copy link
Contributor

@stof stof commented Apr 16, 2020

When a failure happens in a parallel task, it is totally normal that other tasks have not reported completion, as gulp.parallel will report the error without waiting for other tasks.

Closes #203
Closes #162

}

function errorFunction() {
throw new Error('Error!');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're not completing this task, cb(new Error('Error!')).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fine, the domains will catch it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I thought too. Our documentation makes it clear synchronous tasks aren't supported, and throwing an error is technically synchronous.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the test. But the effect is the same. An exception thrown synchronously is already catched and considered as a failure completion.

@phated
Copy link
Member

phated commented Apr 16, 2020

This solution seems solid but give me a bit to think on it.

Should be able to ship this weekend.

@stof stof force-pushed the sync_task_error branch from fd0d3de to 0e70ad0 Compare April 16, 2020 21:00
@stof
Copy link
Contributor Author

stof commented Apr 17, 2020

test test failure on node 0.10 looks unrelated to my change.

@stof
Copy link
Contributor Author

stof commented Apr 17, 2020

and indeed, the latest build on master has even more such timeout failures.

@stof
Copy link
Contributor Author

stof commented Apr 20, 2020

@phated did you have a chance to think about it ?

@phated
Copy link
Member

phated commented Apr 20, 2020

@stof thanks for the reminder! Actually, something came to mind this morning: I think we need to support the --continue flag within this change. If someone specifies the --continue flag, we want to continue to report errors and sync tasks because the CLI will won't exit on the first error. Does that make sense?

I'm not sure how hard it will be to thread that flag through the system. It can be specified in the config file too, so we have to thread it from the main function.

@stof
Copy link
Contributor Author

stof commented Apr 20, 2020

I would prefer if you could work on adding support for --continue yourselves. I fear I don't know the project good enough to figure out how to get this flag value.

@stof
Copy link
Contributor Author

stof commented Apr 24, 2020

@phated AFAIU, even in --continue mode, there is no guarantee that stop or error will be called for each task. Errors are delayed until everything is settled. But then, as single reporting is done in case of an error.

@stof stof force-pushed the sync_task_error branch from 0e70ad0 to 78e64d2 Compare April 27, 2020 12:43
@stof
Copy link
Contributor Author

stof commented Apr 27, 2020

@phated PR updated to account for --continue, with 2 new tests to ensure we don't regress on these.
Fortunately, detecting this was quite simple, as the sync-tasks logic runs after that process.env.UNDERTAKER_SETTLE is updated based on all possible config, so I could read it (as undertaker does), without having to pass it around explicitly.

@phated
Copy link
Member

phated commented Apr 27, 2020

@stof thanks, looking at this today.

@stof
Copy link
Contributor Author

stof commented May 19, 2020

@phated any news on this ?

When a failure happens in a parallel task, it is totally normal that
other tasks have not reported completion, as gulp.parallel will report
the error without waiting for other tasks.
@stof stof force-pushed the sync_task_error branch from 78e64d2 to 2157118 Compare May 19, 2020 10:37
@phated
Copy link
Member

phated commented May 19, 2020

@stof I've been really swamped recently. Sorry about that.

I will try to find some time coming up. Thanks for the ping.

@phated phated merged commit 8de7b2a into gulpjs:master May 20, 2020
@phated
Copy link
Member

phated commented May 20, 2020

Thanks for hanging in there! I know it's hard to stay motivated when we take awhile to respond. I'm going to get this out tonight once CI finishes.

@stof stof deleted the sync_task_error branch May 20, 2020 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants