You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "Roiocam (via GitHub)" <gi...@apache.org> on 2023/12/28 07:57:26 UTC
[PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Roiocam opened a new pull request, #887:
URL: https://github.com/apache/incubator-pekko/pull/887
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#issuecomment-1891918320
> stray println comments
@pjfanning Do you want to recheck this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1456030339
##########
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 =
Review Comment:
@Roiocam @mdedetrich I think when we are not using @inline and iniline def, we may just online this method？but I think the jvm should do the same after some iterations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1456517025
##########
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 =
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1441216168
##########
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 = {
+ 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)
Review Comment:
There is another viewpoint on this: "It is fine because it will always be at the top of the stack frame." I have made an example to verify it, see the another comment on below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1437465652
##########
actor/src/main/scala/org/apache/pekko/actor/Scheduler.scala:
##########
@@ -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`)
Review Comment:
Woops, Thanks, it is done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1437461802
##########
actor/src/main/scala/org/apache/pekko/actor/Scheduler.scala:
##########
@@ -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`)
Review Comment:
what's wrong, i thinkg it is good to have a clarity javadoc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1437459923
##########
actor/src/main/scala/org/apache/pekko/actor/Scheduler.scala:
##########
@@ -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`)
Review Comment:
can you undo this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#issuecomment-1870966133
I checked both Netty and Java, which will throw a IllegalArgumentException.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1441212739
##########
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)
Review Comment:
I think `checkPeriod` is clearer than `checkDelay`, delay may be misunderstand to initialDelay.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#issuecomment-1896078163
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1437519178
##########
actor/src/main/scala/org/apache/pekko/actor/LightArrayRevolverScheduler.scala:
##########
@@ -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")
Review Comment:
what about "Task scheduled with [${delay.toSeconds}] seconds delay, which means creating an infinite loop. The expected delay must be greater than 0."?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1437500370
##########
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 = {
+ 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)
Review Comment:
make sense, this is not something that wroth to abstract, use inline we could gain less stack frame
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1456519591
##########
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 =
Review Comment:
Yes, we can do that in a separate pr, I have to say, this is a hot path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1437922161
##########
actor/src/main/scala/org/apache/pekko/actor/LightArrayRevolverScheduler.scala:
##########
@@ -105,12 +105,20 @@ class LightArrayRevolverScheduler(config: Config, log: LoggingAdapter, threadFac
override def scheduleWithFixedDelay(initialDelay: FiniteDuration, delay: FiniteDuration)(runnable: Runnable)(
implicit executor: ExecutionContext): Cancellable = {
+ 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.")
Review Comment:
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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#issuecomment-1870939428
> @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, as the CPU is a preemptive resource, so 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1441197953
##########
actor/src/main/scala/org/apache/pekko/actor/LightArrayRevolverScheduler.scala:
##########
@@ -105,12 +105,20 @@ class LightArrayRevolverScheduler(config: Config, log: LoggingAdapter, threadFac
override def scheduleWithFixedDelay(initialDelay: FiniteDuration, delay: FiniteDuration)(runnable: Runnable)(
implicit executor: ExecutionContext): Cancellable = {
+ 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.")
Review Comment:
You are right about the compiler's knowledge, but this change is mostly about reducing the stack frame.
```diff
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 an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1441212739
##########
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)
Review Comment:
I think `checkPeriod` is clarify than `checkDelay`, delay may be misunderstand to initialDelay.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#issuecomment-1891954796
> > stray println comments
>
> @pjfanning Do you want to recheck this?
the printlns are commented out still but the lines should be removed altogether
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1456517025
##########
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 =
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1437462664
##########
actor/src/main/scala/org/apache/pekko/actor/Scheduler.scala:
##########
@@ -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`)
Review Comment:
It's the `.` that was removed - can you add that back?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#issuecomment-1870931013
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1437562053
##########
actor-tests/src/test/scala/org/apache/pekko/actor/SchedulerSpec.scala:
##########
@@ -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")
Review Comment:
remove the printlns
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1457246154
##########
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 =
Review Comment:
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1457241187
##########
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)
Review Comment:
done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#issuecomment-1898329051
@Roiocam could you create a backport PR (to 1.0.x branch)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#issuecomment-1870963920
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
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#issuecomment-1871049490
I will go to airport, will try to review this when free
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1437918334
##########
actor/src/main/scala/org/apache/pekko/actor/LightArrayRevolverScheduler.scala:
##########
@@ -105,12 +105,20 @@ class LightArrayRevolverScheduler(config: Config, log: LoggingAdapter, threadFac
override def scheduleWithFixedDelay(initialDelay: FiniteDuration, delay: FiniteDuration)(runnable: Runnable)(
implicit executor: ExecutionContext): Cancellable = {
+ 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.")
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1437498056
##########
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 = {
+ 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)
Review Comment:
I think we should add the check to `scheduleAtFixedRate` and `scheduleWithFixedDelay`.
And inline the method,otherwise it will add a stackframe
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#issuecomment-1892107918
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1456512688
##########
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)
Review Comment:
How about test with the length field directly？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1456520966
##########
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 =
Review Comment:
Hotspot Compiler is very good at optimising this. There is zero evidence that this code needs excess optimisation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1437563326
##########
actor/src/main/scala/org/apache/pekko/actor/LightArrayRevolverScheduler.scala:
##########
@@ -105,12 +105,20 @@ class LightArrayRevolverScheduler(config: Config, log: LoggingAdapter, threadFac
override def scheduleWithFixedDelay(initialDelay: FiniteDuration, delay: FiniteDuration)(runnable: Runnable)(
implicit executor: ExecutionContext): Cancellable = {
+ 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.")
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1437500503
##########
actor/src/main/scala/org/apache/pekko/actor/LightArrayRevolverScheduler.scala:
##########
@@ -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")
Review Comment:
use:
- `delay :xxx expected > 0` or `delay should > 0`.
user do not know what's `delayNanos`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1441202071
##########
actor/src/main/scala/org/apache/pekko/actor/LightArrayRevolverScheduler.scala:
##########
@@ -105,12 +105,20 @@ class LightArrayRevolverScheduler(config: Config, log: LoggingAdapter, threadFac
override def scheduleWithFixedDelay(initialDelay: FiniteDuration, delay: FiniteDuration)(runnable: Runnable)(
implicit executor: ExecutionContext): Cancellable = {
+ 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.")
Review Comment:
This is acceptable, even in real-world case, where the dispatcher stack frame is only a few. while other frames are more like noise.
```log
[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://bank-account@127.0.0.1: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)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#issuecomment-1898216255
@pjfanning Do you want to re-review it now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#issuecomment-1898330275
Thank you @Roiocam
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning merged PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1456517025
##########
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 =
Review Comment:
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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: reject zero and negative periodic tasks schedule [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #887:
URL: https://github.com/apache/incubator-pekko/pull/887#discussion_r1456030339
##########
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 =
Review Comment:
@Roiocam @mdedetrich I think when we are not using @inline and iniline def, we may just online this method？but I think the jvm should do the same after some iterations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org