-
-
Notifications
You must be signed in to change notification settings - Fork 146
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
[RFC] Change cancellation semantics #56
Comments
To me, abort and don't care are actually different from a client perspective. I can think of many cases where I require don't care semantics and some where I require abort, but these are distinct. |
Could you specify a use case where you require abort semantics? |
Sure. The most obvious case for me is during our code deployments where we abort a number of long running operations using |
Not sure we're having the same definition of "abortion" in this context. The new semantics still have abortion in the sense that the root promise can register a cancellation callback for cleanup tasks or something. The difference is, that the promise won't be resolved on cancellation. Bluebird automatically rejected the promises in their 2.x, that's why they talk about "abortion". |
I think that matches my assumption of what abortion and don't care mean. Allow me to elaborate on my previous point. When we deploy code, and at various other times, we gracefully shutdown our worker processes. When we do this, there are a number of promises which we abort before awaiting their resolution. In these cases it is important to await resolution of these promises because we don't want to terminate the process while certain tasks are still running. Once the various promises have been resolved we terminate the process. I.e., we do care about the resolution of the tasks which these promises represent. To me, abortion and don't care actually seem very distinct in the sense that I wouldn't be very comfortable with unknowingly aborting distant promises, which might still be referenced elsewhere in my application, simply because a combinator promise was resolved, for example. I don't really like the fact that Bluebird provides no distinction here, but I do appreciate that you want to learn from their lessons when you make your own design decisions. |
I'm still not sure we're talking about the same, especially because you opened #55 which more or less implements the don't care semantics by deregistering the handlers. You can still resolve a promise after cancellation and handlers registered via |
This can be considered a good thing (provider is free to choose either) or it could be considered a bad thing (unpredictable consumer behavior).
Do we have any other promise implementations that support cancellation? The reason why I'm asking is because the bluebird docs explicitly say "cancellation feature is by default turned off".
I think this is a valid concern. However, I consider this a rather weak argument if it comes down to performance only.
Not sure how useful this is. Personally, I would rather not mix this and discuss this after discussing the desired cancellation semantics.
This looks like the main issue here :) |
Let's take a look at a rather simple promise producer here: https://github.com/clue/php-promise-stream-react#buffer. The Its cancellation behavior is currently undocumented, but implemented:
I think we can all agree on its internal behavior. The "cancellation handler" should be called once NOBODY is interested in a promise result anymore. This matches with the current implementation and afaict nobody is asking to change this. However, the external behavior is slightly more tricky. In the above example, it makes sense to reject the promise with an exception - with no access to the buffered contents. However, one could also use the current cancellation semantics to resolve the promise with the (partial) buffered contents so far (for example, only buffer for x seconds and then cancel the buffering process). Whether this is actually useful is up for debate. |
I'd like to define first what a promise is: it is simply a placeholder for the return value of a function. So, when using promises, you must not use the promises to implement any logic, there are other, better patterns to implement this, like Rx or streams. This is one of the reasons we've removed the progression API. You can compare this with Consequently, the cancellation stuff should also be removed because you could argue it is also out of the scope of promises :)
For example guzzle/promises and icicle support cancellation but simply reject the promise. I'm not sure about the reason why it is turned off by default in bluebird, but i suspect it'd be for the reasons i stated above.
Fully agree here. At the moment i tend to keep the current signature for BC.
Maybe we can pull @joshdifabio in here again :)
Yes.
See the intro of this comment. This is nothing you should implement with promises. This would be a clear candidate for Rx or streams. |
I think we all agree here :) And given its async nature, it (unlike its sync counterparts) also makes sense to
This seems to be the culprit here. Predictable behavior is certainly desired. This is not currently enforced and afaict we could enforce this without breaking BC. To me, "cancellation" implies a signal to the producer of the promise: "I don't care anymore, you're free to stop / clean up the whole operation". As a consumer of the promise I do not expect to get a successful fulfillment value of the promise thereafter: $p = operation();
$p->cancel();
$p->then($never); For consistency, I probably still expect a rejection with the message "operation canceled": $p = operation();
$p->cancel();
$p->then($never, $once); Note however that this only applies to pending promises. Promises that are already successfully fulfilled should probably keep ignoring cancellation: $p = Promise\resolve(42);
$p->cancel();
$p->then($once, $never); We could easily enforce this by invoking the cancellation handler as usual and then rejecting the promise if it is still pending. We can discourage calling the The good thing about this is that it can be considered a feature addition and does not necessarily involve a BC break. Also, enforcing the promise to be settled will invoke any pending callbacks and then invoke its normal cleanup procedure.
Afaict both implement the above logic, i.e. "reject after invoking cancellation". Bonus points for consistency with other implementations 👍
You're not actually suggesting this, right? :) |
I'm a little bit puzzled now as i'm not quite sure what you're suggesting exactly :) Note, that it matters if you call $p = operation();
$p->then($never, $never);
$p->always($once);
$p->cancel();
$p->then($never, $once); // $once is called with a CancellationException
$p->always($once); Calling $promise = new Promise(function(callable $resolve, callable $reject, callable $onCancel) {
$onCancel(function() use ($reject) {
$reject(); // This has no effect!
});
}); If we keep the old signature, we either remove the parameters for the canceller callback or replace them with no-op callbacks for BC. $promise = new Promise(function(callable $resolve, callable $reject) {
}, function(callable $resolve, callable $reject) {
$reject(); // No-op
}); The behaviour of resolved promises is unchanged. $p = Promise\resolve(42);
$p->then($once, $never);
$p->cancel(); // Ignored
$p->then($once, $never);
Nope :) |
Oh, I just wanted to suggest that. A promise is a placeholder value, you can't really abort a value. |
That's one of the major problems of cancelling promises (results) instead of operations. You might want to have a look at cancellation tokens and how they're implemented in .NET and Amp. By cancelling the operation instead of the result, you don't have to even care who subscribed to the promise and who might even subscribe in the future without the promise knowing it yet. |
I'd like to move forward with this one as this is kind of an old issue that hasn't seen any activity in a while and it's unlikely this will get traction any time soon. The project has changed significantly in the meantime and it looks like this deals with a number of issues that might have been addressed in the meantime already.
In particular, it looks like part of the original motivation was memory optimizations that have been addressed in #113, #115, #116, #117, #118, #119, #123, #124 and others already.
The suggested constructor would incur a significant BC break with an unclear upgrade path for consumers of this package and no clear benefits outlined so far. While I don't think the suggested signature is bad per se, I don't think it's worth the effort at the moment until we find a better reason for doing so.
Cancellation tokens are an okay alternative to the cancellation support we have imho. I agree promise cancellation isn't perfect, but we've used this for countless of projects ever since and have yet to run into any major problems. If we want to follow up on this suggestion, I would suggest bringing this up as a separate feature request as this is clearly out of scope for the upcoming promise version as it would incur a very significant BC break with an unclear upgrade path for consumers of this package. I'm closing this for now as it hasn't received any input in a while and I believe this has been answered. If you feel this is still relevant or there's anything else worth discussing, I'm happy to follow up on this either by reopening this or in separate tickets that can be linked against this one. 👍 Thank you for the discussion, keep it up! 👍 |
This issue serves a basis for discussing changing the cancellation semantics. The change is best described by quoting the bluebird docs:
At the moment, there is no predictable behavior when cancelling promises because the producer of the root promise decides what the behaviour is (eg. rejecting the promises).
Changing the semantics would also allow for internal performance and memory optimizations because registered handlers could be discarded (see #55).
New Promise constructor and resolver function signature:
Note, that the second
$canceller
argument has been removed from the Promise constructor. Everything is now handled inside the resolver function.Advantage: Resolution and cancellation share the same scope and multiple cancellation callbacks can be registered via
$onCancel
.Possible problem: The third
$onCancel
argument has been the$notify
callback in 2.x. This might lead to subtle bugs when upgrading from 2.x to 3.x.Handlers
Handlers registered with
always()
are called even if the promise is cancelled as it is thefinally
counterpart from try/catch. No other types of handlers (registered withthen()
,otherwise()
etc.) will be called in case of cancellation.Consuming cancelled promises will return a promise that is rejected with a
CancellationException
as the rejection reason.The text was updated successfully, but these errors were encountered: