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.

Sign up for GitHub

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

fix: reject zero and negative periodic tasks schedule #887

Merged
merged 6 commits into from
Jan 18, 2024
Merged

fix: reject zero and negative periodic tasks schedule #887

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0e0a31f
fix: reject zero and negative periodic tasks schedule
Roiocam Dec 28, 2023
12f1038
fix: undo the symbol change
Roiocam Dec 28, 2023
d261148
use different test name, redescribe the exception
Roiocam Dec 28, 2023
8683ea1
abstract check function
Roiocam Jan 4, 2024
30a40ab
remove the printlns change
Roiocam Jan 17, 2024
b4530f1
reduce time units scale convert
Roiocam Jan 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 57 additions & 2 deletions actor-tests/src/test/scala/org/apache/pekko/actor/SchedulerSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -661,6 +661,45 @@ class LightArrayRevolverSchedulerSpec extends PekkoSpec(SchedulerSpec.testConfRe
}
}

"using scheduleAtFixedRate" must {
"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)
}

"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 {
"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)
}

"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 {
Expand All @@ -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)
}
}
}

Expand Down Expand Up @@ -713,14 +770,12 @@ class LightArrayRevolverSchedulerSpec extends PekkoSpec(SchedulerSpec.testConfRe
@volatile var time: Long = start
val sched = new LightArrayRevolverScheduler(config.withFallback(system.settings.config), log, tf) {
override protected def clock(): Long = {
// println(s"clock=$time")
time
}

override protected def getShutdownTimeout: FiniteDuration = (10 seconds).dilated

override protected def waitNanos(ns: Long): Unit = {
// println(s"waiting $ns")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the printlns

prb.ref ! ns
try time += (lbq.get match {
case q: LinkedBlockingQueue[Long] => q.take()
Expand Down
7 changes: 7 additions & 0 deletions actor/src/main/scala/org/apache/pekko/actor/LightArrayRevolverScheduler.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,14 @@ class LightArrayRevolverScheduler(config: Config, log: LoggingAdapter, threadFac

override def scheduleWithFixedDelay(initialDelay: FiniteDuration, delay: FiniteDuration)(runnable: Runnable)(
implicit executor: ExecutionContext): Cancellable = {
checkZeroPeriod(delay.toNanos)
checkMaxDelay(roundUp(delay).toNanos)
super.scheduleWithFixedDelay(initialDelay, delay)(runnable)
}

override def schedule(initialDelay: FiniteDuration, delay: FiniteDuration, runnable: Runnable)(
implicit executor: ExecutionContext): Cancellable = {
checkZeroPeriod(delay.toNanos)
checkMaxDelay(roundUp(delay).toNanos)
new AtomicCancellable(InitialRepeatMarker) { self =>
final override protected def scheduledFirst(): Cancellable =
Expand Down Expand Up @@ -200,6 +202,11 @@ class LightArrayRevolverScheduler(config: Config, log: LoggingAdapter, threadFac
s"Task scheduled with [${delayNanos.nanos.toSeconds}] seconds delay, " +
s"which is too far in future, maximum delay is [${(tickNanos * Int.MaxValue).nanos.toSeconds - 1}] seconds")

private def checkZeroPeriod(delayNanos: Long): Unit =
if (delayNanos <= 0)
throw new IllegalArgumentException(
"Task scheduled with zero or negative period, which is create an an infinite loop")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use:

  • delay :xxx expected > 0 or delay should > 0.

user do not know what's delayNanos

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about "Task scheduled with [${delay.toSeconds}] seconds delay, which means creating an infinite loop. The expected delay must be greater than 0."?


private val stopped = new AtomicReference[Promise[immutable.Seq[TimerTask]]]
private def stop(): Future[immutable.Seq[TimerTask]] = {
val p = Promise[immutable.Seq[TimerTask]]()
Expand Down
20 changes: 10 additions & 10 deletions actor/src/main/scala/org/apache/pekko/actor/Scheduler.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,8 @@ 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
* reach (calculated as: `delay / tickNanos > Int.MaxValue`).
* @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.
*/
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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.
Expand All @@ -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.
Expand All @@ -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.
Expand Down