Add optional timeout parameter to await*() methods

[joshdifabio]

Calling await*() with no timeout is scary. I think that in almost all cases, when we wait for something we don't want to potentially wait forever. While clients of this library could handle this themselves, it's much easier to add it to this library directly.

Regarding the implementation: it might make sense to have a TimeoutException class to make it easier for clients to differentiate between rejected promises and timeouts.

It's totally cool if you don't want to merge this!

[clue]

Thanks for your PR and the great suggestion 👍
The feature makes perfect sense to me, so I'd love to see this in.

Some remarks about the current implementation which should be addressed first.

• The timer should be cancel()'ed when the promise(s) settle.
  A second call to the await*() method should not be affected by a timer that was previously set up.
  This also eases interoperability with mixed async and non-async projects.
• All pending promises should be cancel()'ed when the timer is invoked.
  Debatable, but IMHO the "await" implies that we either wait for the promises to settle or will never care about their (possible) future results. Arguably, another use case would be "checking" whether a promises settles within a given time frame, but this should probably be handled in a different feature?
• Add a dedicated TimeoutException (extends RuntimeException) as per your above suggestion
• Introduces some (more) duplication which I'd like to avoid

Looking forward to getting this in :)

[joshdifabio]

The timer should be cancel()'ed when the promise(s) settle. A second call to the await*() method should not be affected by a timer that was previously set up. This also eases interoperability with mixed async and non-async projects.

This just occurred to me after pushing :). I've fixed this now and added tests to cover it. I went for a different approach to invoking cancel() just because it meant adding less code, but I'm happy to instead invoke cancel() if you'd prefer.

[joshdifabio]

All pending promises should be cancel()'ed when the timer is invoked. Debatable, but IMHO the "await" implies that we either wait for the promises to settle or will never care about their (possible) future results. Arguably, another use case would be "checking" whether a promises settles within a given time frame, but this should probably be handled in a different feature?

This is indeed an interesting and debatable issue. I do feel that it's a little restrictive to cancel() any remaining promises upon a timeout as obviously there might be other consumers of that promise, but I do appreciate the benefits and evidently awaitRace() and awaitAll() are implemented in such a way that they will cancel any unresolved promises upon completing. And it would be best to be consistent with what you've already implemented. Have you considered implementing an Exception class with a cancelPromises() method or similar, or otherwise making that 'auto cancel' functionality optional?

Introduces some (more) duplication which I'd like to avoid

Agreed. I'll take a look at that at some point.

[joshdifabio]

@clue I've updated the PR to implement each of the recommendations you made in the OP but I haven't yet added tests to prove that promises are cancelled when a timeout occurs. I've also re-factored a bit to shorten each of the public methods and reduce code duplication.

I seem to have broken the HHVM build on Travis; I'll take a look at that now.

[joshdifabio]

Any thoughts on this @clue? I'm currently depending on my fork for another project so it'd be great to get this merged. If there are any remaining issues then I can look at them this evening.

[clue]

Have you considered implementing an Exception class with a cancelPromises() method or similar, or otherwise making that 'auto cancel' functionality optional?

This is an interesting idea… Let's move this to a dedicated ticket (#4), okay? :-)

Any thoughts on this @clue?

Thanks for poking me :-) I'll review this again shortly in detail (and would appreciate if anybody else happens to have some thoughts on this!). At first sight, I'm not quite sure I like the code layout. The original code was rather clean, but did contain quite a bit of duplicate code. The new code avoids this duplication, at the cost of readability (all IMHO).

Either way, thanks for your PR and the discussion!

[joshdifabio]

Thanks for poking me :-) I'll review this again shortly in detail (and would appreciate if anybody else happens to have some thoughts on this!).

Thanks :)

At first sight, I'm not quite sure I like the code layout. The original code was rather clean, but did contain quite a bit of duplicate code. The new code avoids this duplication, at the cost of readability (all IMHO).

The trouble is, there's a lot of duplication and code complexity otherwise. Maybe the approach (naming, at least) can be improved, but I think it'd be a shame to essentially copy & paste 50 loc into all three methods.

[clue]

Thanks for the update @joshdifabio! Unfortunately I haven't had the time to review this more thoroughly so far :( I'll keep an eye on this and will come back to this as time permits!

In the meantime, have you checked out my (alpha quality) clue/promise-timeout project? This might be an alternative approach that could remove quite a bit of the code duplication.

[joshdifabio]

Thanks for the update.

I've had a look at the promise-timeout package; thanks for that. I have a few projects kicking around which depend on the timeout fork of php-block-react for unit tests. At some point I'll try rewriting those tests to use php-block-react in combination with promise-timeout and see which approach yields the most pleasing results.

Keep up the good work!

[clue]

[…] use php-block-react in combination with promise-timeout and see which approach yields the most pleasing results

Awesome, please let me know how it works out! 👍

I still think that adding a timeout to blocking functions is a common feature that should be exposed in a way that is easy to use. Though I'm curious as to how this is best achieved. Thanks for your patience and much appreciated work so far!

[clue]

@joshdifabio Thanks for all the effort you've put into this ticket! 🎉

I very much appreciate your work and I'd love to get this functionality in! 👍

Given that the new functional API landed the other day, I currently don't see how we could resolve the resulting merge conflict. Instead, I'd like to close this ticket for now, so that it serves as an archive for the history of the original implementation idea.

If you (or anybody else for that matter) happens to want to rewrite this on top of the functional API, please feel invited to file a new PR and I'd be happy to help push this forward 👍 I've just opened #14 for the reference so this doesn't get lost.

Thanks!