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 all commits
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
57 changes: 57 additions & 0 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 {
"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 {
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
8 changes: 8 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 = {
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 =
Expand Down Expand Up @@ -193,6 +195,12 @@ class LightArrayRevolverScheduler(config: Config, log: LoggingAdapter, threadFac
task
}

private def checkPeriod(delay: FiniteDuration): Unit =
Copy link
Contributor

@mdedetrich mdedetrich Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think @He-Pin made a comment here before which now seems to be gone regarding inlining.

I just wanted to say you can inline this method if you want, you just have to make a scala-2 version using the @inline annotation and a scala-3 version with the inline keyword.

The easiest way to do this would be to place the checkPeriod function into a private[pekko] trait in the scala-2/scala-3 source folders respectively and make class LightArrayRevolverScheduler extend that trait.

It might be best to do this in a separate PR though, since iirc we are wanting to backport this to 1.0.x and 1.0.x doesn't have the scala-2 inliner available.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we can do that in a separate pr, I have to say, this is a hot path.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hotspot Compiler is very good at optimising this. There is zero evidence that this code needs excess optimisation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hotspot Compiler is very good at optimising this. There is zero evidence that this code needs excess optimisation.

I am also skeptical but it can be proved with benchmarks in a separate PR if need be

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
Expand Down
18 changes: 9 additions & 9 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,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.
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
Loading