Skip to content
Permalink
Browse files

[JENKINS-28690] Aha! So I believe this will fully resolve any of thes…

…e kinds of deadlocks

- Without this, then it becomes a question of find catch and release for each potential code path that
  might end up restoring the interrupt flag on the current thread.
- Since standard Lock support is kind enough to restore the interrupt flag on the current thread
  when blocked waiting for the lock, that would be a hiding to nothing
- I welcome others to review my logic detailed in the code comment
- I am leaving the code comment as this is IMHO too important to assume that somebody will
  check the git commit history
  • Loading branch information...
stephenc committed Aug 6, 2015
1 parent d331c12 commit 0ba505b60ca86d6b103b070a690a98ae6fef8c5d
Showing with 28 additions and 1 deletion.
  1. +28 −1 core/src/main/java/hudson/model/Executor.java
@@ -142,7 +142,34 @@ public Executor(@Nonnull Computer owner, int n) {

@Override
public void interrupt() {
interrupt(Result.ABORTED);
if (Thread.currentThread() == this) {
// If you catch an InterruptedException the "correct" options are limited to one of two choices:
// 1. Propagate the exception;
// 2. Restore the Thread.currentThread().interrupted() flag
// The JVM locking support assumes such behaviour.
// Evil Jenkins overrides the interrupt() method so that when a different thread interrupts this thread
// we abort the build.
// but that causes JENKINS-28690 style deadlocks when the correctly written code does
//
// try {
// ... some long running thing ...
// } catch (InterruptedException e) {
// ... some tidy up
// // restore interrupted flag
// Thread.currentThread().interrupted();
// }
//
// What about why we do not set the Result.ABORTED on this branch?
// That is a good question to ask, the answer is that the only time a thread should be restoring
// its own interrupted flag is when that thread has already been interrupted by another thread
// as such we should assume that the result has already been applied. If that assumption were
// incorrect, then the Run.execute's catch (InterruptedException) block will either set the result
// or have been escaped - in which case the result of the run has been sealed anyway so it does not
// matter.
super.interrupt();
} else {
interrupt(Result.ABORTED);
}
}

void interruptForShutdown() {

0 comments on commit 0ba505b

Please sign in to comment.
You can’t perform that action at this time.