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

Adds the possibility to cancel all callbacks of a promise #50

Closed
wants to merge 3 commits into from
Closed

Adds the possibility to cancel all callbacks of a promise #50

wants to merge 3 commits into from

Conversation

raycoarana
Copy link

@raycoarana raycoarana commented Feb 6, 2015

This method try to help apps developers that wan't to prevent leaks of Activity/Fragment when a user is leaving them and a promise is pending to be completed. Removing all callbacks let the Activity/Fragment to be garbage collected.


This change is Reviewable

…t leak classes when leaving the app and those callbacks are no more needed.
@saturnism
Copy link
Member

hiya - which version of jdeferred are you using? the latest should've been patched where finished promises would've cleared all the callbacks.

@raycoarana
Copy link
Author

The case what I see we need this is when the Promise is not already finish
but the user want to leave the screen (the activity or fragment un
Android). If we don't clear the callbacks at that moment, the screen will
be leaked until the promise finishes.

@Wattos
Copy link

Wattos commented Oct 7, 2015

I would 👍 the request for a cancel() method on a promise. However, this changeset does not seem complete, as ideally the creator of a deferred object would be able to register on cancel and do something. As an example, consider having a promise for an API Http request. Ideally, you would like to be able to cancel the request as well.

@saturnism
Copy link
Member

agreed - I do feel we can implement cancel if and only if the underlying task is also cancelable. But, a promise is also immutable. What do you feel about making Deferred object cancelable?

@lkho
Copy link

lkho commented Apr 20, 2016

Hi everyone, how is this going on? I'm waiting for it to be merged :)
Maybe we can create a subclass for CancellableDeferred and CancellablePromise since the original meaning of deferred and promise do not contain a cancel mechanism.

@saturnism
Copy link
Member

hi @raycoarana! just getting back to this, I'm thinking of renaming the method to something else. p.cancel() feels like we are canceling the promise/task, rather than cleaning up the handlers/callbacks.

Would you be able to suggest another name?

@raycoarana
Copy link
Author

Sure, maybe clearCallbacks() is clear enough. What do you think?

@lkho lkho mentioned this pull request May 2, 2016
@saturnism saturnism added this to the 1.3 milestone Nov 26, 2016
@saturnism saturnism self-assigned this Nov 26, 2016
@saturnism saturnism removed this from the 1.3 milestone Oct 28, 2017
@saturnism
Copy link
Member

/cc @aalmiray we don't have ways to clear callbacks. But, having clear on a Promise can be contentious too, given that a promise can have multiple consumers. One could accidentally clear another's callback.

@saturnism saturnism removed their assignment Oct 28, 2017
@saturnism saturnism removed this from 1.3.x in Release Dashboard Oct 28, 2017
@aalmiray
Copy link
Contributor

it looks to me that the actual problem is that the task may still be running, not the fact that the promise has at least one callback registered. If this is the case then we must find a way to cancel tasks submitted via dm.when() when the first task rejects a value, similarly as it's done with dm.race(). Be aware that automatic cancellation of tasks can only happen when the submitted type is not a Promise but a Runnable/Callable/DeferredRunnable/DeferredCallable/DeferredFutureTask/Future/DeferredAsyncTask.

@raycoarana raycoarana closed this Jan 18, 2021
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.

None yet

5 participants