Avoid retrying if the build is aborted #134
Conversation
| } catch (InterruptedIOException e) { | ||
| throw e; |
There was a problem hiding this comment.
I'm curious on what's the purpose of this. Perhaps it can be removed now.
/cc @dwnusbaum
There was a problem hiding this comment.
There are no more references in the code about it:
Perhaps it was meant to do what this PR actually does, and therefore could be removed.
There was a problem hiding this comment.
Out of curiosity, the checkout to get Jenkinsfile step does the same thing:
And it is probably affected by the same problem.
There was a problem hiding this comment.
And it is probably affected by the same problem.
I just confirmed here, it is. I'll craft a similar fix there.
There was a problem hiding this comment.
You would have to look through all implementations of SCM in other plugins and see if their checkout methods ever throw InterruptedIOException (keeping in mind that they might not throw it directly, but might use IO-related JDK APIs that in turn throw InterruptedIOException) to really know if it is unused or not.
Really, looking at jenkinsci/workflow-cps-plugin#147, I think the better fix would be to change the catch (Exception e) block here to catch (IOException e), so that instances of InterruptedException (which includes FlowInterruptedException) always bypass the retry logic, and then add a catch block for AbortException. In other words, use the exact same try/catch logic as in jenkinsci/workflow-cps-plugin#147.
There was a problem hiding this comment.
Oh, but then yes jenkinsci/workflow-cps-plugin#274 made workflow-cps have the same bad behavior. And then compare the third implementation over in https://github.com/jenkinsci/pipeline-groovy-lib-plugin/blob/83f864f66d8f827c442913c4e3f7000c3a52b5fb/src/main/java/org/jenkinsci/plugins/workflow/libs/SCMBasedRetriever.java#L218. Probably we need to create a helper method in workflow-api for this retry logic that handles all of the cases correctly.
There was a problem hiding this comment.
use the exact same try/catch logic as in jenkinsci/workflow-cps-plugin#147.
But I just confirmed that one doesn't work either. It's affected by the same issue.
There was a problem hiding this comment.
IMO the logic seems like it should be like this: instances of InterruptedIOException and InterruptedException should bypass retries, except we need to check FlowInterruptedException, which is a subclass of InterruptedException, specially, and only skip retries if isActualInterruption().
Then, for any other Exception, we would retry, handling instances of AbortException specially as needed to avoid printing stack traces for them.
There was a problem hiding this comment.
Probably we need to create a helper method in workflow-api for this retry logic that handles all of the cases correctly.
I totally agree with that. But this is kind of already done by the RetryStep:
RetryStep works fine. It doesn't present this issue.
It would be nice if it could be reused here, but I am super inexperienced working with Jenkins plugins. I would appreciate if someone else can implement it, but please let me know if there is no intention in doing so.
|
Ok, this worked, no retries were executed and the build ended up aborted:
However, I don't get why it still prints "Checkout failed" and "Retrying after 10 seconds", as I re-throw the exception before reaching these lines. |
|
I'm not confident on merging this. I'd rather let some maintainer handle it. |
Refs #110 (comment)
Refs #110 (comment)
Testing done
This change was not tested yet, and I could not figure out how to make an automated test for it.
I'll let you know once I have tested it.
Submitter checklist