fix: reject zero and negative periodic tasks schedule #887

Roiocam · 2023-12-28T07:57:23Z

create a zero periodic tasks on scheduler will let scheduler busy and can not scheduler other task.

on jdk ScheduledThreadPoolExecutor, the zero period task currently is forrbidden.

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

pjfanning · 2023-12-28T08:21:05Z

@Roiocam have you seen the busy loop behaviour in the real world? What about the case where some sets a very short value (eg 1) - isn't that almost as bad?

Roiocam · 2023-12-28T08:33:32Z

@Roiocam have you seen the busy loop behaviour in the real world? What about the case where some sets a very short value (eg 1) - isn't that almost as bad?

The issue was discovered in our unit test on CI, and the reason was that our heartbeat configuration was not being loaded correctly.

The "short value" might cause a busy loop, but we always get a chance to rest. However, in the case of a zero period, this can lead to the actor system never being able to shut down.

Please refer to the specific unit test on the link for more details: https://gist.github.com/Roiocam/d317683d54bdbf3afe75b1b945c7f115

pjfanning · 2023-12-28T09:07:15Z

tests fail

seems to be down to a duplicate test name

8T08:21:22.8083608Z [12-28 08:21:22.808] �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31morg.apache.pekko.actor.LightArrayRevolverSchedulerSpec *** ABORTED *** (1 millisecond)�[0m�[0m
2023-12-28T08:21:22.8088023Z [12-28 08:21:22.808] �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m  org.scalatest.exceptions.DuplicateTestNameException was thrown inside "using scheduleWithFixedDelay" must, construction cannot continue: "Duplicate test name: A LightArrayRevolverScheduler when using scheduleWithFixedDelay must reject periodic tasks scheduled with zero interval" (SchedulerSpec.scala:727)�[0m�[0m

He-Pin · 2023-12-28T09:09:59Z

I checked both Netty and Java, which will throw a IllegalArgumentException.

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

He-Pin · 2023-12-28T09:16:08Z

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

+  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")


use:

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

user do not know what's delayNanos

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

He-Pin · 2023-12-28T10:48:30Z

I will go to airport, will try to review this when free

pjfanning · 2023-12-28T10:59:46Z

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

        time
      }

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

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


remove the printlns

pjfanning

stray println comments

pjfanning · 2023-12-28T11:02:18Z

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

+    if (delay.toNanos <= 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.")


I'm not going to block this over this - but I would I strongly argue that this should be a function - like checkMaxDelay.

I'm not convinced by all this inline debate. Modern JIT compilers are good - in fact, very good - at this. I struggle to accept that this coding pratcice is justified without someone proving it to me.

Since this is an exception it shouldn't be evaluated in the hot path anyways, or is this an exception which is expected to be thrown and caught in the standard business logic flow?

the pre-existing line right after this does a similar check and throws the same type of exception - checkMaxDelay(roundUp(delay).toNanos)

I have no problem with validating the values here.

The scaladoc for this method already has @throws java.lang.IllegalArgumentException

You are right about the compiler's knowledge, but this change is mostly about reducing the stack frame.

java.lang.IllegalArgumentException: Task scheduled with [0] seconds delay, which means creating an infinite loop. The expected delay must be greater than 0. + at org.apache.pekko.actor.LightArrayRevolverScheduler.checkDelay(LightArrayRevolverScheduler.scala:205) at org.apache.pekko.actor.LightArrayRevolverScheduler.scheduleWithFixedDelay(LightArrayRevolverScheduler.scala:108) at org.apache.pekko.actor.Scheduler.scheduleWithFixedDelay(Scheduler.scala:157) at org.apache.pekko.actor.Scheduler.scheduleWithFixedDelay$(Scheduler.scala:149) at org.apache.pekko.actor.LightArrayRevolverScheduler.scheduleWithFixedDelay(LightArrayRevolverScheduler.scala:50) at org.apache.pekko.actor.LightArrayRevolverSchedulerSpec.$anonfun$new$52(SchedulerSpec.scala:688)

It seems like everything is okay because most of the time we only care about the top frame of the stack.

This is acceptable, even in real-world case, where the dispatcher stack frame is only a few. while other frames are more like noise.

[2024-01-04 10:09:59,876] [ERROR] [ispatcher-55] [akka.actor.typed.Behavior$ ]: Supervisor StopSupervisor saw failure: Task scheduled with [42949672] seconds delay, which is too far in future, maximum delay is [21474835] seconds MDC: {akkaAddress=akka://[email protected]:2551, akkaSource=akka://bank-account/system/sharding/xxxxxxxxxx, sourceActorSystem=bank-account} java.lang.IllegalArgumentException: Task scheduled with [42949672] seconds delay, which is too far in future, maximum delay is [21474835] seconds at akka.actor.LightArrayRevolverScheduler.checkMaxDelay(LightArrayRevolverScheduler.scala:216) at akka.actor.LightArrayRevolverScheduler.scheduleWithFixedDelay(LightArrayRevolverScheduler.scala:98) at akka.actor.typed.internal.adapter.SchedulerAdapter.scheduleWithFixedDelay(SchedulerAdapter.scala:44) at akka.actor.typed.internal.TimerSchedulerImpl.startTimer(TimerSchedulerImpl.scala:127) at akka.actor.typed.internal.TimerSchedulerImpl.startTimerWithFixedDelay(TimerSchedulerImpl.scala:101) at akka.actor.typed.internal.TimerSchedulerCrossDslSupport.startTimerWithFixedDelay(TimerSchedulerImpl.scala:63) at akka.actor.typed.internal.TimerSchedulerCrossDslSupport.startTimerWithFixedDelay$(TimerSchedulerImpl.scala:62) at akka.actor.typed.internal.TimerSchedulerImpl.startTimerWithFixedDelay(TimerSchedulerImpl.scala:83) at akka.actor.typed.javadsl.BuiltBehavior.receive(BehaviorBuilder.scala:197) at akka.actor.typed.javadsl.BuiltBehavior.receive(BehaviorBuilder.scala:186) at akka.actor.typed.Behavior$.interpret(Behavior.scala:274) at akka.actor.typed.Behavior$.interpretMessage(Behavior.scala:230) at akka.actor.typed.internal.InterceptorImpl$$anon$2.apply(InterceptorImpl.scala:57) at akka.actor.typed.internal.InterceptorImpl.receive(InterceptorImpl.scala:87) at akka.actor.typed.Behavior$.interpret(Behavior.scala:274) at akka.actor.typed.Behavior$.interpretMessage(Behavior.scala:230) at akka.actor.typed.internal.InterceptorImpl$$anon$2.apply(InterceptorImpl.scala:57) at akka.actor.typed.internal.SimpleSupervisor.aroundReceive(Supervision.scala:131) at akka.actor.typed.internal.InterceptorImpl.receive(InterceptorImpl.scala:85) at akka.actor.typed.Behavior$.interpret(Behavior.scala:274) at akka.actor.typed.Behavior$.interpretMessage(Behavior.scala:230) at akka.actor.typed.internal.adapter.ActorAdapter.handleMessage(ActorAdapter.scala:128) at akka.actor.typed.internal.adapter.ActorAdapter.aroundReceive(ActorAdapter.scala:107) at akka.actor.ActorCell.receiveMessage(ActorCell.scala:579) at akka.actor.ActorCell.invoke(ActorCell.scala:547) at akka.dispatch.Mailbox.processMailbox(Mailbox.scala:270) at akka.dispatch.Mailbox.run(Mailbox.scala:231) at akka.dispatch.Mailbox.exec(Mailbox.scala:243) at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289) at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056) at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692) at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:175)

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

mdedetrich · 2024-01-15T11:03:17Z

stray println comments

@pjfanning Do you want to recheck this?

pjfanning · 2024-01-15T11:15:24Z

stray println comments

@pjfanning Do you want to recheck this?

the printlns are commented out still but the lines should be removed altogether

mdedetrich · 2024-01-15T12:43:05Z

the printlns are commented out still but the lines should be removed altogether

@Roiocam Do you want to remove this so we can get the PR over the finish line?

Roiocam · 2024-01-17T15:42:59Z

the printlns are commented out still but the lines should be removed altogether

@Roiocam Do you want to remove this so we can get the PR over the finish line?

Done, let's keep this pull request simple.

He-Pin

Lgtm

mdedetrich · 2024-01-17T21:41:22Z

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

@@ -193,6 +195,12 @@ class LightArrayRevolverScheduler(config: Config, log: LoggingAdapter, threadFac
      task
    }

+  private def checkPeriod(delay: FiniteDuration): Unit =


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.

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

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

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

He-Pin

lgtm

mdedetrich · 2024-01-18T10:34:03Z

@pjfanning Do you want to re-review it now?

pjfanning

lgtm

pjfanning · 2024-01-18T11:48:04Z

@Roiocam could you create a backport PR (to 1.0.x branch)?

He-Pin · 2024-01-18T11:48:57Z

Thank you @Roiocam

* 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

fix: reject zero and negative periodic tasks schedule

0e0a31f

pjfanning reviewed Dec 28, 2023

View reviewed changes

actor/src/main/scala/org/apache/pekko/actor/Scheduler.scala Outdated Show resolved Hide resolved

fix: undo the symbol change

12f1038

pjfanning added this to the 1.0.x milestone Dec 28, 2023

He-Pin reviewed Dec 28, 2023

View reviewed changes

actor/src/main/scala/org/apache/pekko/actor/LightArrayRevolverScheduler.scala Outdated Show resolved Hide resolved

Roiocam marked this pull request as draft December 28, 2023 09:14

He-Pin reviewed Dec 28, 2023

View reviewed changes

use different test name, redescribe the exception

d261148

Roiocam marked this pull request as ready for review December 28, 2023 09:46

pjfanning reviewed Dec 28, 2023

View reviewed changes

pjfanning requested changes Dec 28, 2023

View reviewed changes

pjfanning reviewed Dec 28, 2023

View reviewed changes

abstract check function

8683ea1

Roiocam commented Jan 4, 2024

View reviewed changes

actor/src/main/scala/org/apache/pekko/actor/LightArrayRevolverScheduler.scala Show resolved Hide resolved

remove the printlns change

30a40ab

He-Pin approved these changes Jan 17, 2024

View reviewed changes

mdedetrich reviewed Jan 17, 2024

View reviewed changes

reduce time units scale convert

b4530f1

He-Pin approved these changes Jan 18, 2024

View reviewed changes

pjfanning approved these changes Jan 18, 2024

View reviewed changes

pjfanning merged commit 7b2a3d8 into apache:main Jan 18, 2024
18 checks passed

Roiocam deleted the reject-zero-periodic-tasks branch January 19, 2024 01:57

Roiocam mentioned this pull request Jan 19, 2024

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

Merged

pjfanning mentioned this pull request Apr 25, 2024

Nightly pekko 1.1 tests apache/pekko-persistence-jdbc#181

Merged

gclendinning02 mentioned this pull request Jul 5, 2024

InMemoryReadJournal functionality broken by Pekko v1.0.3 alstanchev/pekko-persistence-inmemory#10

Closed

tiagomota mentioned this pull request Sep 11, 2024

Update to Pekko to 1.1.0 SwissBorg/pekko-persistence-postgres#59

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: reject zero and negative periodic tasks schedule #887

fix: reject zero and negative periodic tasks schedule #887

Roiocam commented Dec 28, 2023

pjfanning commented Dec 28, 2023

Roiocam commented Dec 28, 2023 •

edited

Loading

pjfanning commented Dec 28, 2023

He-Pin commented Dec 28, 2023

He-Pin Dec 28, 2023

Roiocam Dec 28, 2023

He-Pin commented Dec 28, 2023

pjfanning Dec 28, 2023

pjfanning left a comment

pjfanning Dec 28, 2023

mdedetrich Dec 28, 2023

pjfanning Dec 28, 2023

Roiocam Jan 4, 2024

Roiocam Jan 4, 2024

mdedetrich commented Jan 15, 2024

pjfanning commented Jan 15, 2024

mdedetrich commented Jan 15, 2024

Roiocam commented Jan 17, 2024

He-Pin left a comment

mdedetrich Jan 17, 2024 •

edited

Loading

He-Pin Jan 17, 2024

pjfanning Jan 17, 2024

mdedetrich Jan 18, 2024

He-Pin left a comment

mdedetrich commented Jan 18, 2024

pjfanning left a comment

pjfanning commented Jan 18, 2024

He-Pin commented Jan 18, 2024

fix: reject zero and negative periodic tasks schedule #887

fix: reject zero and negative periodic tasks schedule #887

Conversation

Roiocam commented Dec 28, 2023

pjfanning commented Dec 28, 2023

Roiocam commented Dec 28, 2023 • edited Loading

pjfanning commented Dec 28, 2023

He-Pin commented Dec 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

He-Pin commented Dec 28, 2023

Choose a reason for hiding this comment

pjfanning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdedetrich commented Jan 15, 2024

pjfanning commented Jan 15, 2024

mdedetrich commented Jan 15, 2024

Roiocam commented Jan 17, 2024

He-Pin left a comment

Choose a reason for hiding this comment

mdedetrich Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

He-Pin left a comment

Choose a reason for hiding this comment

mdedetrich commented Jan 18, 2024

pjfanning left a comment

Choose a reason for hiding this comment

pjfanning commented Jan 18, 2024

He-Pin commented Jan 18, 2024

Roiocam commented Dec 28, 2023 •

edited

Loading

mdedetrich Jan 17, 2024 •

edited

Loading