Repository navigation
[Suggestion] Add taskified variants of timeout combinators - #838
Conversation
|
I'm 👍 on adding these, given we have Ref and TaskLocal stuff, these can be useful. |
|
Cool. Any suggestions on names? I admit that |
|
Another question - since timeout is a task now, should I try to recover somehow? If a timeout task fails, then the whole task fails. Not sure if this is the desired behaviour. |
|
As far as naming goes, I'd suggest keeping L as strictly a suffix. I.e. The issue I'm currently seeing is that currently you're waiting for a duration to compute, and then also for that duration. Maybe you should subtract the time it took to compute the duration from actual delay? I don't think sensible recovery is possible. You can't just ignore the delay, as that might cause resource leak. You can return the result if it managed to compute before the delay failed, but I think that should be already done by race (although a test wouldn't hurt). |
|
Good catch on the evaluation time. I changed the code to account for it but now is seems like some users would want to actually ignore that time. Should I put it under a flag? def timeoutL(timeout: Task[Duration], respectTimeoutTimings: Boolean = true) = ??? |
| * | ||
| * Example: | ||
| * {{{ | ||
| * import cats.effect.IO |
| * to a value of `5 seconds`, then, | ||
| * if the value of this parameter is `true`, | ||
| * this task will timeout in exactly 5 seconds | ||
| * from the momemnt computation started; |
There was a problem hiding this comment.
| * from the momemnt computation started; | |
| * from the moment computation started; |
| * | ||
| * Example: | ||
| * {{{ | ||
| * import cats.effect.IO |
There was a problem hiding this comment.
| * import cats.effect.IO |
| * this will timeout in 5 seconds _after_ | ||
| * the evaluation of timeout task was completed. | ||
| **/ | ||
| final def timeoutToL[B >: A](after: Task[FiniteDuration], |
There was a problem hiding this comment.
Could you please change formatting like this:
final def timeoutToL[B >: A](
after: Task[FiniteDuration],
backup: Task[B],
respectTimeoutTimings: Boolean = true): Task[B] = | } | ||
|
|
||
| test("Task#timeoutL considers time taken to evaluate the duration task") { implicit s => | ||
| // "3 seconds" after delay of 2s |
There was a problem hiding this comment.
I don't think we need the comments
In other places we use different methods and call it |
|
We have So, I think having one version is good enough (and I'm not a huge fan of boolean flags tbh). We don't need to address every use case with one method, if we have enough reasonably convenient composable pieces to let user solve the problem. Which one should be default is the next question. I have a vague feeling that subtracting elapsed time would be less surprising, but I'm not insisting on it. |
|
@tkroman Revert scalafmt please, I think we were undecided whether to use it |
|
Done. Feel free to merge :) |
* Add taskified variants of timeout combinators * add tests * account for timeout evaluation time * add scaladoc * Address PR comments * Update Task.scala
I propose adding
timeoutL/timeoutLTovariants oftimeout/timeoutTo. Motivating examples are in the code. If you think this deserves to be in the library, I'll add tests etc.