Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(core): Use AbortController to notify nodes to abort execution #6141
fix(core): Use AbortController to notify nodes to abort execution #6141
Changes from 15 commits
09d3f87
d276b4c
bd28ab2
df6ae1e
029c951
43c954d
ee8669c
7ee3a30
31a296f
4b9a419
b7dfa0d
0a34be8
9f9c7a4
3f1a3ee
7a736da
12a506e
3e4a646
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once this PR is merged, I'm going to get rid of
PCancelable
and use a regular promise withAbortController
instead.Even
p-cancelable
recommends to do that since 3 years.Moving away from
PCancelable
should also make this code more readable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we can pass in a
reason
arg - all cancellations that can happen right now are user-requested? Else we might want to use this arg to differentiate between user-requested cancellations vs. cancellations initiated on our end?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. we can have cancellation from user interaction, timeout, or server shutdown.
I've created a backlog ticket: ENG-92.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For understanding - We are resolving this
PCancelable
inside a timeout in order to allow for other promises to resolve first, i.e. to force this promise to be placed in the macrotask queue, processed after the microtask queue. Is this correct? Else why are we doing this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. we have a bunch of async operations (that don't really take any time, and possibly don't even need to be async) that need to happen between the cancellation, and before this
resolve
is called. I don't like this, and plan to fix this in the same PR where I'll removePCancelable
completely.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're using
canceled
internally butstopped
externally - we might want to unify for consistency.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that we should unify these. Created a ticket ENG-93