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: cancelled instances hanging indefinitely #98

Merged
merged 6 commits into from
Jun 21, 2024
Merged

Conversation

paoloricciuti
Copy link
Collaborator

@paoloricciuti paoloricciuti commented Jun 7, 2024

Closes #93 and probably #94 too

Pretty nasty bug: when cancelled we need to resolve the promise in some way or it will hang indefinitely. This was shown in task types that were waiting for the old one to finish but it was a general bug.

For the moment i've opted into rejecting the promise with a Custom Error so that the user can check against that instance if the task has been cancelled. This feels the right thing to do but i would love your opinion on this when you are back from vacation @beerinho @nickschot

I also realized that we were doing something wrong with the restart so i moved the array to be a set and removed that specific instance instead of popping the array.

@paoloricciuti paoloricciuti linked an issue Jun 7, 2024 that may be closed by this pull request
@paoloricciuti paoloricciuti added the bug Something isn't working label Jun 7, 2024
@nickschot
Copy link
Member

If I understand correctly, this is what's known as a cancellation error in ember-concurrency. There's a util available called didCancel to allow the user to check that. https://github.com/machty/ember-concurrency/blob/master/packages/ember-concurrency/src/-private/external/task-instance/cancelation.js#L23

@paoloricciuti
Copy link
Collaborator Author

If I understand correctly, this is what's known as a cancellation error in ember-concurrency. There's a util available called didCancel to allow the user to check that. https://github.com/machty/ember-concurrency/blob/master/packages/ember-concurrency/src/-private/external/task-instance/cancelation.js#L23

Yes...i thought of adding an helper too but at the end of the day the user can always check with instanceof. We can provide the helper too but maybe i would do it in a separate PR.

@beerinho
Copy link
Collaborator

Yes...i thought of adding an helper too but at the end of the day the user can always check with instanceof. We can provide the helper too but maybe i would do it in a separate PR.

I prefer the idea of having a helper attached to the task instance over the requirement to import the CancellationError and checking if it's an instanceof. I've created a task for it here - #101

@paoloricciuti paoloricciuti merged commit a3dd6b1 into main Jun 21, 2024
4 checks passed
@paoloricciuti paoloricciuti deleted the cancellation-bug branch June 21, 2024 08:35
@github-actions github-actions bot mentioned this pull request Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cancellation breaks enqueue and drop tasks
3 participants