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

Author: Roiocam

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

@@ -105,12 +105,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)
     new AtomicCancellable(InitialRepeatMarker) { self =>
       final override protected def scheduledFirst(): Cancellable =
@@ -193,6 +195,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 java.lang.IllegalArgumentException if the given delays exceed the maximum
+   * @throws java.lang.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.
@@ -116,7 +116,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 java.lang.IllegalArgumentException if the given delays exceed the maximum
+   * @throws java.lang.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.
@@ -215,7 +215,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 java.lang.IllegalArgumentException if the given delays exceed the maximum
+   * @throws java.lang.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.
@@ -251,7 +251,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 java.lang.IllegalArgumentException if the given delays exceed the maximum
+   * @throws java.lang.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.
@@ -415,7 +415,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 java.lang.IllegalArgumentException if the given delays exceed the maximum
+   * @throws java.lang.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.
@@ -433,7 +433,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 java.lang.IllegalArgumentException if the given delays exceed the maximum
+   * @throws java.lang.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.
@@ -452,7 +452,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 java.lang.IllegalArgumentException if the given delays exceed the maximum
+   * @throws java.lang.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.
@@ -466,7 +466,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 java.lang.IllegalArgumentException if the given delays exceed the maximum
+   * @throws java.lang.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.
@@ -477,7 +477,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 java.lang.IllegalArgumentException if the given delays exceed the maximum
+   * @throws java.lang.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.