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

[Suggestion] Add taskified variants of timeout combinators #838

Merged
merged 7 commits into from Apr 1, 2019

Conversation

Projects
None yet
3 participants
@tkroman
Copy link
Contributor

commented Mar 6, 2019

I propose adding timeoutL/timeoutLTo variants of timeout/timeoutTo. Motivating examples are in the code. If you think this deserves to be in the library, I'll add tests etc.

@oleg-py oleg-py requested a review from alexandru Mar 8, 2019

@oleg-py

This comment has been minimized.

Copy link
Collaborator

commented Mar 8, 2019

I'm 👍 on adding these, given we have Ref and TaskLocal stuff, these can be useful.

@tkroman

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

Cool. Any suggestions on names? I admit that timeoutLTo is far from being nicely discoverable except by signature.

@tkroman

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

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.

@oleg-py

This comment has been minimized.

Copy link
Collaborator

commented Mar 9, 2019

As far as naming goes, I'd suggest keeping L as strictly a suffix. I.e. timeoutToL.

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).

@tkroman

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2019

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

This comment has been minimized.

Copy link
@Avasil

Avasil Mar 24, 2019

Collaborator

reduntant import I think

@Avasil

Avasil approved these changes Mar 24, 2019

Copy link
Collaborator

left a comment

Thank you @tkroman :)
I've left few minor docs/formatting related comments but it looks good to me overall

* 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;

This comment has been minimized.

Copy link
@Avasil

Avasil Mar 24, 2019

Collaborator
Suggested change
* from the momemnt computation started;
* from the moment computation started;
*
* Example:
* {{{
* import cats.effect.IO

This comment has been minimized.

Copy link
@Avasil

Avasil Mar 24, 2019

Collaborator
Suggested change
* 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],

This comment has been minimized.

Copy link
@Avasil

Avasil Mar 24, 2019

Collaborator

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

This comment has been minimized.

Copy link
@Avasil

Avasil Mar 24, 2019

Collaborator

I don't think we need the comments

@Avasil

This comment has been minimized.

Copy link
Collaborator

commented Mar 24, 2019

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) = ???

In other places we use different methods and call it fixedRate and fixedDelay but I think in this case flag is reasonable. WDYT @oleg-py ?

@oleg-py

This comment has been minimized.

Copy link
Collaborator

commented Mar 24, 2019

We have task.timed. If we have one of these versions, the other one is always constructible via timed + flatMap + either subtraction or addition, depending on variant 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 added some commits Mar 29, 2019

@tkroman

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

@Avasil @oleg-py addressed comments & merged. I also ran scalafmt on Task.scala which lead to a larger diff, should I revert?

@Avasil

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2019

@tkroman Revert scalafmt please, I think we were undecided whether to use it

@tkroman

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Done. Feel free to merge :)

@oleg-py oleg-py merged commit 2243ea7 into monix:master Apr 1, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.