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

Prelude timeout does not use the HasTime<RT> runtime trait #1012

Open
timmi-on-rails opened this issue Mar 17, 2022 · 3 comments
Open

Prelude timeout does not use the HasTime<RT> runtime trait #1012

timmi-on-rails opened this issue Mar 17, 2022 · 3 comments
Assignees
Labels
enhancement v4 Related to version < 5.0.0 v5 Related to version 5.0.0+

Comments

@timmi-on-rails
Copy link
Contributor

I was surprised that the runtime-dependent timeout implementation does not utilize the HasTime<RT> trait for the internal timeout task.

Current version

// signature (see https://github.com/louthy/language-ext/blob/main/LanguageExt.Core/Effects/Aff/Prelude/Aff.Prelude.cs#L228)
public static Aff<RT, A> timeout<RT, A>(TimeSpan timeoutDelay, Aff<RT, A> ma) where RT : struct, HasCancel<RT>

// timeout task implementation (see https://github.com/louthy/language-ext/blob/main/LanguageExt.Core/Effects/Aff/Aff.cs#L174)
var delay = Task.Delay(timeoutDelay, delayTokSrc.Token);

What I expected

// signature
public static Aff<RT, A> timeout<RT, A>(TimeSpan timeoutDelay, Aff<RT, A> ma) where RT : struct, HasCancel<RT>, HasTime<RT>

// timeout task implementation
// something like...
var delay = ?...? env.TimeEff.Map(timeIO => timeIO.SleepFor(timeoutDelay, delayTokSrc.Token)) ?...?

Is my expectation unusual?

(If not, any change is probably a breaking change, so it comes down to extending the method documentation and maybe provide a timeout2 function)

@louthy
Copy link
Owner

louthy commented Mar 23, 2022

@timmi-on-rails I don't think it's unreasonable to think that, however, I don't think it adds much in the way of tangible benefits here. A couple of reasons I think this:

  • There's only really one valid approach to timing out the underlying Task, so an injectable behaviour doesn't add anything.
  • It's a core capability. All Affs should support timeout and retry strategies.

There could be an argument, which I think is fair, that by making this use HasTime for its delay-task, we could allow time to slow down or pause. That could certainly have some benefits, although even that would need some very careful coding of the timeout behaviour ... not impossible, but of debateable value.

@timmi-on-rails
Copy link
Contributor Author

@louthy I fully agree to your second point, but only partially to your first point. While there is only one valid way to timeout in a Live Environment, there are still alternative valid implementations how to timeout in a test environment. Here is some context:

I have written a chat bot, which uses timeouts when waiting for user responses and then sends a reminder to answer the bot's message.
Now I would like to write some tests.
My plan was to time-travel with the HasTime Test Runtime implementation and speed up test durations.
This way I don't have to wait 5 minutes for the timeout until the test passes.

@louthy
Copy link
Owner

louthy commented Mar 29, 2022

@timmi-on-rails That's a reasonable request, let me have a think 👍

@louthy louthy self-assigned this Oct 22, 2024
@louthy louthy added enhancement v5 Related to version 5.0.0+ v4 Related to version < 5.0.0 labels Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement v4 Related to version < 5.0.0 v5 Related to version 5.0.0+
Projects
None yet
Development

No branches or pull requests

2 participants