Copilot commented on code in PR #3022:
URL: https://github.com/apache/pekko/pull/3022#discussion_r3330378917
##########
actor/src/main/scala/org/apache/pekko/actor/FaultHandling.scala:
##########
@@ -544,7 +544,8 @@ case class AllForOneStrategy(
* across actors and thus this field does not take up much space
*/
private val retriesWindow =
- (maxNrOfRetriesOption(maxNrOfRetries),
withinTimeRangeOption(withinTimeRange).map(_.toMillis.toInt))
+ (maxNrOfRetriesOption(maxNrOfRetries),
+ withinTimeRangeOption(withinTimeRange).map(d =>
math.min(Int.MaxValue.toLong, d.toMillis).toInt))
Review Comment:
This change fixes the overflow bug for large `withinTimeRange`, but there’s
no regression test covering a duration whose `toMillis` exceeds `Int.MaxValue`
(e.g. 30.days). Adding a test that verifies restart limiting still works for
such a duration would prevent reintroducing the silent negative-window behavior.
##########
actor/src/main/scala/org/apache/pekko/util/FrequencySketch.scala:
##########
@@ -61,7 +61,7 @@ private[pekko] object FrequencySketch {
depth: Int = 4,
counterBits: Int = 4)(implicit hasher: Hasher[A]): FrequencySketch[A] = {
val width = widthMultiplier * Bits.ceilingPowerOfTwo(capacity)
- val resetSize = (resetMultiplier * capacity).toInt
+ val resetSize = math.min(Int.MaxValue.toLong, (resetMultiplier *
capacity).toLong).toInt
new FrequencySketch(depth, width, counterBits, resetSize, hasher)
Review Comment:
`widthMultiplier * Bits.ceilingPowerOfTwo(capacity)` is still computed in
`Int`, so for large capacities (e.g. capacity > 536,870,912 with the default
widthMultiplier=4) this can overflow to 0/negative and then pass `isPowerOfTwo`
(which returns true for 0), leading to incorrect masks/array sizes. Since this
PR is already hardening overflow behavior, it would be good to compute `width`
using a `Long` intermediate and fail fast (or clamp) if the result is outside
the valid positive `Int` range.
##########
actor/src/main/scala/org/apache/pekko/actor/FaultHandling.scala:
##########
@@ -656,7 +657,8 @@ case class OneForOneStrategy(
*/
private val retriesWindow = (
SupervisorStrategy.maxNrOfRetriesOption(maxNrOfRetries),
-
SupervisorStrategy.withinTimeRangeOption(withinTimeRange).map(_.toMillis.toInt))
+ SupervisorStrategy.withinTimeRangeOption(withinTimeRange).map(d =>
+ math.min(Int.MaxValue.toLong, d.toMillis).toInt))
Review Comment:
Same as above: please add a regression test for `withinTimeRange` values
whose `toMillis` exceeds `Int.MaxValue` (e.g. 30.days) to ensure the
retry-window logic doesn’t silently degrade to “no window” due to an `Int`
overflow.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
