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

Do not cancel promise once timeout fires #9

Closed
clue opened this issue Jul 2, 2015 · 7 comments
Closed

Do not cancel promise once timeout fires #9

clue opened this issue Jul 2, 2015 · 7 comments

Comments

@clue
Copy link
Member

clue commented Jul 2, 2015

Currently, the promise will be cancelled (edit: rejected) once the timeout fires. This is debatable, but IMHO the name "timeout" implies that we either wait for the promise to settle within the timeout or will never care about its (possible) future result.

Arguably, another use case would be "checking" whether a promise settles within a given time frame. I'd like to put this out here to see if anybody happens to have any thoughts and/or actual use cases.

Also refs clue/reactphp-block#4

@clue
Copy link
Member Author

clue commented Jul 3, 2015

I think these are actually two independent use cases that should be clearly separated.

As such, I'd vote against a boolean flag (boolean trap?) and vote for two distinct methods similar to this:

timeout($promise, $time, $loop);
timeoutWillCancel($promise, $time, $loop);

The default should probably not cancel any promises, as this is the default promise behavior (e.g. Promise\all()). (Warning, BC break ahead).

@jsor
Copy link
Member

jsor commented Jul 3, 2015

Currently, the promise will be rejected once the timeout fires

Should this read as "...the promise will be cancelled once..."? As i understand it, the returned promise will be always rejected, but the cancel() call on the original promise is the difference between the 2 functions.

👍 for 2 functions instead of a boolean.

@clue
Copy link
Member Author

clue commented Jul 3, 2015

Should this read as "...the promise will be cancelled once..."?

Indeed, thanks for spotting, I've fixed the typo in the OP :)

Other than that, thanks for the feedback! Looks like we have the same understanding.

👍 for 2 functions instead of a boolean.

I've toyed around with this but have yet to come up with some decent function names, so perhaps you happen to have any ideas?

The original name used to be Timeout\await() and a second Timeout\awaitOrCancel() would make sense. Now that the original function is being renamed to Timer\timeout() (see #10), I'm unsure how to name the second one?

@jsor
Copy link
Member

jsor commented Jul 3, 2015

IMHO, the default behaviour (Timer\timeout()) should be cancellation of the promise. This is similar to the "timeout" behaviour i'm used to (ie. for HTTP requests, database/socket connections etc.).

To be honest, i can't think of any use cases for not cancelling a promise. Maybe go with timeout only and add another function later if there are requests for it.

@clue
Copy link
Member Author

clue commented Jul 3, 2015

To be honest, i can't think of any use cases for not cancelling a promise.

Well, I guess I must have been overthinking this?! :) Admittedly, all my use cases actually also assume the canceling behavior.

Thanks for chiming in, very much appreciated!

I have a working prototype for this locally, but will postpone this for now, until we have a valid use case.

@clue
Copy link
Member Author

clue commented Jul 3, 2015

Edit: This is quite intertwined with #3. I have accidentally posted some comments here, that have now been moved over instead.

@clue
Copy link
Member Author

clue commented Jul 4, 2015

I think we agree here. I guess the confusion can be cleared up by providing some documentation (see #14).

I'd like to make this ticket actionable. IMO we should focus on the main use case (as discussed above) here and close the ticket now that we agree on this. The other use case can be addressed in a separate ticket once deemed useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants