fix a rare race condition in treeaction timeout detection #38
Conversation
Iteration 1 introduced a small behavior change that I think we should address. If we hit this break statement (line 798), we are now going to check if we will hit timeout before throwing ActionTimeoutException. I'm leaning towards keeping the original behavior of throwing exception in this case. Example 1) Do not retry, and timeout after 60 seconds. Let's say the Action returned unsuccessfully close to the 60 second timeout. It hits this break statement since no retry policy is set and no continuation is set. We then check if it hits a timeout (new code). We discover that it does/will hit a timeout, so we commit TimeoutOnAction. Example 2) Retry 3 times, then fail. If it times out, continue. In this example, let's say we finish our 3 retries after 55 seconds. We hit this break statement. We then check if it hits a timeout (new code). We discover that it will hit a timeout, since 55 (stopwatch) + 10 (waitTime) > 60 (timeout). Though it is true it would hit a timeout if we waited for another waitTime cycle, but in this case we have already exhausted our retries. It does not make sense to check timeout logic after we know we've hit retry exhausted. #Closed Refers to: Forge.TreeWalker/src/TreeWalkerSession.cs:798 in 3173bfb. [](commit_id = 3173bfb, deletion_comment = False) |
Considering solutions to this. An easy way forward I believe would be to duplicate your changes for ContinuationOnRetryExhaustion. Add a check outside the while loop, before the timeout check. There may be a more clever way to solve this without having to duplicate the checks. In reply to: 767879309 [](ancestors = 767879309) Refers to: Forge.TreeWalker/src/TreeWalkerSession.cs:798 in 3173bfb. [](commit_id = 3173bfb, deletion_comment = False) |
| return; | ||
| } | ||
| } | ||
|
|
||
| // Retries are exhausted. Throw ActionTimeoutException with executeAction exception as innerException. |
There was a problem hiding this comment.
Retries are exhausted [](start = 15, length = 21)
Let's update this message as well since it should be hitting timeouts and not retries exhausted. #Resolved
No description provided.