=act #17140 FSM: forMax(Inf) can now override stateTimeout #17152
Conversation
|
LGTM Shoud this be backported to 2.3? |
|
I think so, it can be considered a bug - in the docs we say this should work. |
|
Test PASSed. |
| } | ||
|
|
||
| currentState.timeout match { | ||
| case SomeMaxFiniteDuration ⇒ // effectively disable stateTimeout |
There was a problem hiding this comment.
Is this a meaningful distinction given a FiniteDuration of Long.MaxValue - 1?
There was a problem hiding this comment.
Effectively what I need to express "such big timeout that the akka scheduler would blow up anyway".
Thought using MaxValue would be cleanest as marker value. I could dig into the scheduler and find it's max and cut-off anything above it to => "effectively never", but that sounds a bit more magic than picking MaxValue as my marker.
Note: Users are not expected to use it directly, they'd do forMax(Inf) which I encode as this MaxValue marker. (Can't change types, if I could we'd change things to Duration from FiniteDuration and we'd be good).
There was a problem hiding this comment.
this is just a workaround that the original code did not handle Duration.Inf although the forMax method (L146) accepts a Duration. The proper fix would be to change the signature of State to have a timeout of Option[Duration] instead of Option[FiniteDuration], but that would break binary compatiblility. So Konrad introduced this artifical marker that is introduced at L148 when a Duration.Inf is encountered, and which can be stored in the State case class without changing its signature.
There was a problem hiding this comment.
Reason for this marker-hack is the FiniteDuration here: final case class State[S, D](stateName: S, stateData: D, timeout: Option[FiniteDuration], ...
There was a problem hiding this comment.
Makes total sense. Apologies for barging in here.
There was a problem hiding this comment.
Nah, no worries – more often than not such barging in is pretty useful :-)
There was a problem hiding this comment.
but @viktorklang 's comment is valid as in you should put a comment there to explain the "why".
There was a problem hiding this comment.
Yes agreed, I'll add one
There was a problem hiding this comment.
Added a comment explaining the reasoning (and todo - we can fix it once we break compat).
|
LGTM |
942cf6c to
8ae438b
Compare
8ae438b to
bdedb10
Compare
=act #17140 FSM: forMax(Inf) can now override stateTimeout
=act akka#17140 FSM: forMax(Inf) can now override stateTimeout
Resolves #17140 while remaining binary and source compatible