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

Make reject-on-cancel optional for Task Queue tasks #2033

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

cwillisf
Copy link
Contributor

@cwillisf cwillisf commented Mar 2, 2019

Proposed Changes

Adjust TaskQueue's do() such that canceling a task does not reject that task's promise. A new doOrReject() method retains the current behavior where the task's promise is rejected on cancel.

Reason for Changes

Using TaskQueue in the micro:bit extension revealed that handling the task's promise rejection is unnecessary in most cases, so having to attach a do-nothing rejection handler is annoying from the perspective of an extension author. Worse, a do-nothing rejection handler might suppress promise rejections due to "real" problems, such as exceptions thrown during task execution.

With the new approach a task can cancel without rejecting the promise, so such a rejection does not need to be suppressed. If an exception is thrown during task execution it will bubble out as a promise rejection and likely cause a browser warning, which is what we want. In those rare cases where the caller might need to react to a task cancel, the caller can use doOrReject instead. In this case the rejection handler should check whether the rejection is due to task cancel or some other exception.

Test Coverage

Coming soon...
(marking this PR as "needs work" pending test coverage)

@cwillisf cwillisf added this to the March 2019 milestone Mar 2, 2019
@cwillisf cwillisf self-assigned this Mar 2, 2019
* @returns {Promise} - a promise for the task's return value.
* @memberof TaskQueue
*/
_do (task, rejectOnCancel, cost) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it look okay for this to be called from outside the file? I think the hardware extensions want to call this function more often than doOrReject

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 was thinking you'd want to call either of these:

taskQueue.do(() => { stuff(); }, someCost);
taskQueue.doOrReject(() => { stuff(); }, someCost).catch(...);

...both of which are exposed currently. If you'd like to call this _do method that's fine, but I expect you'd rather call do() above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry! I didn't see the full context and realize do() was still available, I was not paying attention and only going by the changes here. Thank you for clarifying! Sounds good.

@cwillisf
Copy link
Contributor Author

@ericrosenbaum @evhan55 do you still have any interest in this?

@evhan55
Copy link
Contributor

evhan55 commented May 29, 2019

@cwillisf I am not sure, we are not currently using or have plans to use the TaskQueue in any of the hardware extensions, so it feels safe to move to a Backlog! But @ericrosenbaum might have a better idea..

@evhan55
Copy link
Contributor

evhan55 commented Jun 3, 2019

I am going to unassign myself here as it is unlikely to move forward before my consulting ends.

@evhan55 evhan55 removed their assignment Jun 3, 2019
@chrisgarrity chrisgarrity modified the milestones: March 2019, Schedule Review Jun 1, 2020
@chrisgarrity chrisgarrity removed this from the Schedule Review milestone Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants