From f15a66a2e557459d0a117a9155ccd036ceae5d27 Mon Sep 17 00:00:00 2001 From: AndyChen Date: Thu, 18 Jan 2024 19:47:13 +0800 Subject: [PATCH] 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 --- .../apache/pekko/actor/SchedulerSpec.scala | 57 +++++++++++++++++++ .../actor/LightArrayRevolverScheduler.scala | 8 +++ .../org/apache/pekko/actor/Scheduler.scala | 18 +++--- 3 files changed, 74 insertions(+), 9 deletions(-) diff --git a/actor-tests/src/test/scala/org/apache/pekko/actor/SchedulerSpec.scala b/actor-tests/src/test/scala/org/apache/pekko/actor/SchedulerSpec.scala index fbd31766198..b245d963f39 100644 --- a/actor-tests/src/test/scala/org/apache/pekko/actor/SchedulerSpec.scala +++ b/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) + } } } diff --git a/actor/src/main/scala/org/apache/pekko/actor/LightArrayRevolverScheduler.scala b/actor/src/main/scala/org/apache/pekko/actor/LightArrayRevolverScheduler.scala index 4b9cd82200d..dcd3e4cc96e 100644 --- a/actor/src/main/scala/org/apache/pekko/actor/LightArrayRevolverScheduler.scala +++ b/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 diff --git a/actor/src/main/scala/org/apache/pekko/actor/Scheduler.scala b/actor/src/main/scala/org/apache/pekko/actor/Scheduler.scala index 41abe832e48..95e5832f490 100644 --- a/actor/src/main/scala/org/apache/pekko/actor/Scheduler.scala +++ b/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.