Schedule actions at a given time

[alexandredantas]

Adding functions enabling an effect to run only at a given point in time.
The functions exec and await call each other to establish when an effect must run.

The exec function calls foldM to execute the effect and retrieve its result. At this point, it gives control to await

The await function awaits when schedule signal when it's ready to run. Schedule will always receive. Schedules for this always receive unit as input because they don't need effect to run to decide when to run
When schedule is ready to run, await return control to exec, restarting the process

[alexandredantas]

Two points on this PR:

• i've not added unit tests because i never wrote specs before. I would really appreciate some help to write the tests.
• when building microsite, i got a failure related to jekyll. Was it my fault?

[alexandredantas]

Here i want a suggestion: i'm not considering readiness on run. Should I? If yes, what should be the behavior? Discard results when not ready? Sleep until next readiness?

[jdegoes]

@alexandredantas I think this is a useful feature to have.

However, I think we can do it in a slightly different way that has a cleaner design: adding a method into Scheduler that allows scheduling something at a particular instant.

Then, potentially, every step could return either a delay (relative time) or an instant (absolute time). I have to think this through, but I think this has all the structure required of Schedule and the existing combinators.

Do you think that would work?

[alexandredantas]

@jdegoes make total sense. convert the point at time to a delay and effects would sleep until reaching it. i'll rework on that. thanks for the tip!

Does still make sense keeping scheduled method at ZIO or repeat would be used? I mean, repeat will run effects once before evaluating schedule step but scheduling to run only at a point in time require effects to keep idle (potentially) since beginning.

A very simple way to do it is keeping schedule to check Schedule first and then delegating to repeat, instead duplicating repeat logic. Make sense?

ps: can you help pointing out which properties i should test? :)

[jdegoes]

@alexandredantas The main thing we need to ensure is that invariants around && and || are respected. For example: semantics of && are to choose the larger of the two delays; and ||, to choose the smaller of the two delays.

Things will be easier if we introduce a type:

sealed trait Delay
object Delay {
  final case class Relative(duration: Duration)
  final case class Absolute(time: Instant) // Cannot use Instant for Scala.js???
}

Now our problem is comparing them to see which one comes first requires an effect, because we have to use the Clock, so our comparison methods will change:

sealed trait Delay {
  def < (that: Delay): ZIO[Clock, Nothing, Boolean] = ???
  def <= (that: Delay): ZIO[Clock, Nothing, Boolean] = ???
  def > (that: Delay): ZIO[Clock, Nothing, Boolean] = ???
  def >= (that: Delay): ZIO[Clock, Nothing, Boolean] = ???
  def === (that: Delay): ZIO[Clock, Nothing, Boolean] = ???
}

Direct comparison on Duration happens for && and || methods. It's not desirable to make them do the comparison (instead we should push that all the way to ZIO#retry / ZIO#repeat). So we have to change that by tweaking combineWith, using some new data structure, which can describe comparisons without actually making them:

sealed trait DelayComparison { self =>
  def choose: ZIO[Clock, Nothing, Delay] = 
    self match {
      ??? // Uses effectful comparison methods on `Delay`
    }
}
object DelayComparison {
  final case class Choose(value: Delay) extends DelayComparison
  final case class Min(l: DelayComparison, r: DelayComparison) extends DelayComparison
  final case class Max(l: DelayComparison, r: DelayComparison) extends DelayComparison
}

Now combineWith will not take a function (Duration, Duration) => Duration, but rather, a (Delay, Delay) => DelayComparison. So now Clock dependency (pulled in by comparing Delay) will not contaminate the Schedule methods, but rather be pushed into ZIO#retry / ZIO#repeat, which will call the DelayComparison#choose method.

At this point all existing schedules will be updated to use Delay.Relative (with helper: Delay.relative), since that's what they use now. And some new schedules can use Delay.Absolute, e.g. Schedule.at(...).

[alexandredantas]

@jdegoes Wow!! Such a very nice solution!! I'll come up with something following your suggestion

[alexandredantas]

@jdegoes can you tell me if I'm on the right direction? When I changed Decision.combineWith to (Delay, Delay) => DelayComparison, it was required to touch every other schedule implementation.
Besides, combineWith take Direction.delay as arguments and I have changed it to be DelayComparison, not allowing to apply to the function :(

What did I do wrong?

[alexandredantas]

@jdegoes After banging my head against wall sometimes, I've came up with this implementation. Adjusted tests accordingly and at least build is passing 😄
If things looks good, I want to place a schedule capable of considering only a point in time. However, scalajs doesn't support LocalDateTime. Is there something else I could use to perform date calculations?

[jdegoes]

@alexandredantas After looking at some of the contortions you had to go through, I can see now what I should have seen all alone: Delay and DelayComparison need to be fused into a single structure, called Delay.

sealed trait Delay {
  def run: ZIO[Clock, Nothing, Duration, Instant]
}
object Delay {
  val none: Delay = Relative(Duration.zero)
  // Etc.

  final case class Relative(duration: Duration) extends Delay
  final case class Absolute(instant: Instant) extends Delay // Scala.js compat?
  final case class Min(left: Delay, right: Delay) extends Delay
  final case class Max(left: Delay, right: Delay) extends Delay
|

Now functions can be (Delay, Delay) => Delay and composition will be vastly superior. Should clean up a lot of code, too!

[alexandredantas]

@jdegoes 😄 Sorry for noobiness. I should have tried that first instead implementing them separately. I'll clean up things and do as you suggested. After all, do things look good? Do you see any major mistake?

And about java.time.Instant, scala.js doesn't support it by default (there is an external library) so I thought about using scalaz.Duration pointing to the future. What do you think?

[jdegoes]

@alexandredantas I think a lot of the gnarlier code will be clean after this change. The overall direction of changes seems correct to me. There may be some edge case lodging that needs more careful review when the bigger changes have been made, but you're definitely on the right track! 😄

[alexandredantas]

@jdegoes any update? :)

[jdegoes]

@alexandredantas How about now? 😄

Did you get everything passing???

[alexandredantas]

@jdegoes sorry for not getting the message early. what about friday? anytime :)

About the tests, yes, all passing!! I've merged with master and looks like something was fixed

[jdegoes]

This work is being continued in #1798. @alexandredantas Thank you for all your help on this and if you have some time to contribute, your assistance reviewing #1798 and ensuring we satisfy all requirements of the original ticket would be greatly appreciated. 🙏