fix: reject zero and negative periodic tasks schedule (#887)

* fix: reject zero and negative periodic tasks schedule

* fix: undo the symbol change

* use different test name, redescribe the exception

* abstract check function

* remove the printlns change

* reduce time units scale convert

actor-tests/src/test/scala/org/apache/pekko/actor/SchedulerSpec.scala

@@ -661,6 +661,45 @@ class LightArrayRevolverSchedulerSpec extends PekkoSpec(SchedulerSpec.testConfRe
       }
     }
 
+    "using scheduleAtFixedRate" must {
+      "fixed rate reject periodic tasks scheduled with zero interval" taggedAs TimingTest in {
+        val zeroInterval = 0.seconds
+        import system.dispatcher
+        intercept[IllegalArgumentException] {
+          system.scheduler.scheduleAtFixedRate(100.millis, zeroInterval, testActor, "Not Allowed")
+        }
+        expectNoMessage(1.second)
+      }
+
+      "fixed rate reject periodic tasks scheduled with negative interval" taggedAs TimingTest in {
+        val negativeDelay = -1.seconds
+        import system.dispatcher
+        intercept[IllegalArgumentException] {
+          system.scheduler.scheduleAtFixedRate(100.millis, negativeDelay, testActor, "Not Allowed")
+        }
+        expectNoMessage(1.second)
+      }
+    }
+
+    "using scheduleWithFixedDelay" must {
+      "fixed delay reject periodic tasks scheduled with zero interval" taggedAs TimingTest in {
+        val zeroInterval = 0.seconds
+        import system.dispatcher
+        intercept[IllegalArgumentException] {
+          system.scheduler.scheduleWithFixedDelay(100.millis, zeroInterval, testActor, "Not Allowed")
+        }
+        expectNoMessage(1.second)
+      }
+
+      "fixed delay reject periodic tasks scheduled with negative interval" taggedAs TimingTest in {
+        val negativeDelay = -1.seconds
+        import system.dispatcher
+        intercept[IllegalArgumentException] {
+          system.scheduler.scheduleWithFixedDelay(100.millis, negativeDelay, testActor, "Not Allowed")
+        }
+        expectNoMessage(1.second)
+      }
+    }
     // same tests for fixedDelay and fixedRate
     List(new ScheduleWithFixedDelayAdapter, new ScheduleAtFixedRateAdapter).foreach { scheduleAdapter =>
       s"using $scheduleAdapter" must {
@@ -684,6 +723,24 @@ class LightArrayRevolverSchedulerSpec extends PekkoSpec(SchedulerSpec.testConfRe
           }
           expectNoMessage(1.second)
         }
+
+        "reject periodic tasks scheduled with zero interval" taggedAs TimingTest in {
+          val zeroInterval = 0.seconds
+          import system.dispatcher
+          intercept[IllegalArgumentException] {
+            scheduleAdapter.schedule(100.millis, zeroInterval, testActor, "Not Allowed")
+          }
+          expectNoMessage(1.second)
+        }
+
+        "reject periodic tasks scheduled with negative interval" taggedAs TimingTest in {
+          val negativeInterval = -1.seconds
+          import system.dispatcher
+          intercept[IllegalArgumentException] {
+            scheduleAdapter.schedule(100.millis, negativeInterval, testActor, "Not Allowed")
+          }
+          expectNoMessage(1.second)
+        }
       }
     }
 

actor/src/main/scala/org/apache/pekko/actor/LightArrayRevolverScheduler.scala

@@ -104,12 +104,14 @@ class LightArrayRevolverScheduler(config: Config, log: LoggingAdapter, threadFac
 
   override def scheduleWithFixedDelay(initialDelay: FiniteDuration, delay: FiniteDuration)(runnable: Runnable)(
       implicit executor: ExecutionContext): Cancellable = {
+    checkPeriod(delay)
     checkMaxDelay(roundUp(delay).toNanos)
     super.scheduleWithFixedDelay(initialDelay, delay)(runnable)
   }
 
   override def schedule(initialDelay: FiniteDuration, delay: FiniteDuration, runnable: Runnable)(
       implicit executor: ExecutionContext): Cancellable = {
+    checkPeriod(delay)
     checkMaxDelay(roundUp(delay).toNanos)
     try new AtomicReference[Cancellable](InitialRepeatMarker) with Cancellable { self =>
         compareAndSet(
@@ -218,6 +220,12 @@ class LightArrayRevolverScheduler(config: Config, log: LoggingAdapter, threadFac
       task
     }
 
+  private def checkPeriod(delay: FiniteDuration): Unit =
+    if (delay.length <= 0)
+      throw new IllegalArgumentException(
+        s"Task scheduled with [${delay.toSeconds}] seconds delay, which means creating an infinite loop. " +
+        s"The expected delay must be greater than 0.")
+
   private def checkMaxDelay(delayNanos: Long): Unit =
     if (delayNanos / tickNanos > Int.MaxValue)
       // 1 second margin in the error message due to rounding

actor/src/main/scala/org/apache/pekko/actor/Scheduler.scala

@@ -75,7 +75,7 @@ trait Scheduler {
    * If the `Runnable` throws an exception the repeated scheduling is aborted,
    * i.e. the function will not be invoked any more.
    *
-   * @throws IllegalArgumentException if the given delays exceed the maximum
+   * @throws IllegalArgumentException if the given delays is zero, negative or exceed the maximum
    * reach (calculated as: `delay / tickNanos > Int.MaxValue`).
    *
    * Note: For scheduling within actors `with Timers` should be preferred.
@@ -144,7 +144,7 @@ trait Scheduler {
    * If the `Runnable` throws an exception the repeated scheduling is aborted,
    * i.e. the function will not be invoked any more.
    *
-   * @throws IllegalArgumentException if the given delays exceed the maximum
+   * @throws IllegalArgumentException if the given delays is zero, negative or exceed the maximum
    * reach (calculated as: `delay / tickNanos > Int.MaxValue`).
    *
    * Note: For scheduling within actors `AbstractActorWithTimers` should be preferred.
@@ -243,7 +243,7 @@ trait Scheduler {
    * If the `Runnable` throws an exception the repeated scheduling is aborted,
    * i.e. the function will not be invoked any more.
    *
-   * @throws IllegalArgumentException if the given delays exceed the maximum
+   * @throws IllegalArgumentException if the given delays is zero, negative or exceed the maximum
    * reach (calculated as: `delay / tickNanos > Int.MaxValue`).
    *
    * Note: For scheduling within actors `with Timers` should be preferred.
@@ -279,7 +279,7 @@ trait Scheduler {
    * If the `Runnable` throws an exception the repeated scheduling is aborted,
    * i.e. the function will not be invoked any more.
    *
-   * @throws IllegalArgumentException if the given delays exceed the maximum
+   * @throws IllegalArgumentException if the given delays is zero, negative or exceed the maximum
    * reach (calculated as: `delay / tickNanos > Int.MaxValue`).
    *
    * Note: For scheduling within actors `AbstractActorWithTimers` should be preferred.
@@ -443,7 +443,7 @@ trait Scheduler {
    * Scala API: Schedules a message to be sent once with a delay, i.e. a time period that has
    * to pass before the message is sent.
    *
-   * @throws IllegalArgumentException if the given delays exceed the maximum
+   * @throws IllegalArgumentException if the given delays is zero, negative or exceed the maximum
    * reach (calculated as: `delay / tickNanos > Int.MaxValue`).
    *
    * Note: For scheduling within actors `with Timers` should be preferred.
@@ -461,7 +461,7 @@ trait Scheduler {
    * Java API: Schedules a message to be sent once with a delay, i.e. a time period that has
    * to pass before the message is sent.
    *
-   * @throws IllegalArgumentException if the given delays exceed the maximum
+   * @throws IllegalArgumentException if the given delays is zero, negative or exceed the maximum
    * reach (calculated as: `delay / tickNanos > Int.MaxValue`).
    *
    * Note: For scheduling within actors `AbstractActorWithTimers` should be preferred.
@@ -480,7 +480,7 @@ trait Scheduler {
    * Scala API: Schedules a function to be run once with a delay, i.e. a time period that has
    * to pass before the function is run.
    *
-   * @throws IllegalArgumentException if the given delays exceed the maximum
+   * @throws IllegalArgumentException if the given delays is zero, negative or exceed the maximum
    * reach (calculated as: `delay / tickNanos > Int.MaxValue`).
    *
    * Note: For scheduling within actors `with Timers` should be preferred.
@@ -494,7 +494,7 @@ trait Scheduler {
    * Scala API: Schedules a Runnable to be run once with a delay, i.e. a time period that
    * has to pass before the runnable is executed.
    *
-   * @throws IllegalArgumentException if the given delays exceed the maximum
+   * @throws IllegalArgumentException if the given delays is zero, negative or exceed the maximum
    * reach (calculated as: `delay / tickNanos > Int.MaxValue`).
    *
    * Note: For scheduling within actors `with Timers` should be preferred.
@@ -505,7 +505,7 @@ trait Scheduler {
    * Java API: Schedules a Runnable to be run once with a delay, i.e. a time period that
    * has to pass before the runnable is executed.
    *
-   * @throws IllegalArgumentException if the given delays exceed the maximum
+   * @throws IllegalArgumentException if the given delays is zero, negative or exceed the maximum
    * reach (calculated as: `delay / tickNanos > Int.MaxValue`).
    *
    * Note: For scheduling within actors `AbstractActorWithTimers` should be preferred.