Fix more deadlocks #226
Fix more deadlocks #226
Conversation
…ocks on the execution from the run
|
@abayer Yeah, we had methods in CpsFlowExecution that synchronize on first the |
In general it is pretty fruitless to write automated tests for deadlocks. Possible, just not productive. You are not demonstrating that the code is free of deadlocks, only that the behavior of a particular sequence of events has changed for the better. |
| @@ -620,47 +620,52 @@ private synchronized String getHeadsAsString() { | |||
| * Bypasses {@link #croak(Throwable)} and {@link #onProgramEnd(Outcome)} to guarantee a clean path. | |||
| */ | |||
| @GuardedBy("this") | |||
| synchronized void createPlaceholderNodes(Throwable failureReason) throws Exception { | |||
| this.done = true; | |||
| end.addAction(new ErrorAction(failureReason)); | ||
| head.setNewHead(end); | ||
| } catch (Exception ex) { | ||
| throw ex; |
There was a problem hiding this comment.
Uh, what is this try-catch-throw accomplishing? Just delete it!
There was a problem hiding this comment.
Main reason is to separate out the synchronization components, but also try to ensure the run save happens.
There was a problem hiding this comment.
Wait, no. I'm going to blame jet lag and lack of sleep for this. You're right, nonsensical.
| } | ||
| } | ||
| try { | ||
| saveOwner(); | ||
| } catch (Exception ex) { | ||
| throw ex; |
| try { | ||
| this.execution.saveOwner(); | ||
| } catch (Exception ex) { | ||
| LOGGER.log(Level.WARNING, "Error saving execution for "+this, ex); |
There was a problem hiding this comment.
Useless toString(). Try
LOGGER.log(Level.WARNING, "Error saving execution for " + execution, ex);| @@ -18,6 +18,7 @@ | |||
| import org.codehaus.groovy.control.CompilationUnit; | |||
| import org.codehaus.groovy.control.CompilerConfiguration; | |||
| import org.codehaus.groovy.control.SourceUnit; | |||
| import org.jenkinsci.plugins.workflow.flow.FlowExecution; | |||
| @@ -1217,7 +1226,7 @@ public boolean isComplete() { | |||
| } | |||
|
|
|||
| /** | |||
| * Record the end of the build. | |||
| * Record the end of the build. Note: we should always follow this with a call to saveOwner to persist the result. | |||
See deadlock file from JENKINS-51132 comment.
This prevents deadlocks by ensuring we do not lock Execution before Run (usually by separating the synchronization scopes).
Lock order should always be Run -> Execution when locks are nested, or locks should be separated (the approach generally used here).
Why aren't there testcases? I have low confidence in our ability produce a testcase that replicates all these deadlocks.