New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
API improvements around Executor/Executable #1610
Merged
Merged
Changes from 17 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
f2f988e
[FIXED JENKINS-26090] Executables.getExecutor
jglick a88fb9d
Use ResourceList.EMPTY.
jglick 46a900f
[JENKINS-25938] Introduced AsynchronousExecutable.
jglick 1658b08
Merge branch 'master' into Executable-JENKINS-25938
jglick eb276d8
Added interruptForShutdown flag.
jglick 09f7a2d
[JENKINS-26092] Noting some things that could be improved.
jglick d073f79
Abandoning Executable2; doing everything in AsynchronousExecution.
jglick 8589b21
[FIXED JENKINS-22941] AsynchronousExecution.blocksRestart
jglick bb80340
[FIXED JENKINS-26900] AsynchronousExecution.displayCell
jglick b106ae3
Deleting forgotten reference to Executable2.
jglick d72d5d2
Merge branch 'master' into Executable-JENKINS-25938
jglick ced8d78
As requested by @kohsuke, rename Executables.getExecutor to Executor.of.
jglick c02f516
javadoc formatting fix for better readability from code editor
kohsuke 47c39da
javadoc improvement
kohsuke 4a7c271
More minor javadoc
kohsuke 63d3b26
Fixed a race condition.
kohsuke 546e0ec
Merge pull request #1 from jenkinsci/pull-1610
jglick 0e3b6a9
@GuardedBy as suggested by @stephenc, and some internal Javadoc.
jglick 5f26494
@CheckForNull as suggested by @stephenc.
jglick 86fed62
Addressing some review comments from @stephenc.
jglick File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,13 +60,15 @@ | |
import static java.util.logging.Level.*; | ||
import javax.annotation.CheckForNull; | ||
import javax.annotation.Nonnull; | ||
import jenkins.model.queue.AsynchronousExecution; | ||
import org.kohsuke.accmod.Restricted; | ||
import org.kohsuke.accmod.restrictions.NoExternalUse; | ||
|
||
|
||
/** | ||
* Thread that executes builds. | ||
* Since 1.536, {@link Executor}s start threads on-demand. | ||
* The entire logic should use {@link #isActive()} instead of {@link #isAlive()} | ||
* in order to check if the {@link Executor} it ready to take tasks. | ||
* <p>Callers should use {@link #isActive()} instead of {@link #isAlive()}. | ||
* @author Kohsuke Kawaguchi | ||
*/ | ||
@ExportedBean | ||
|
@@ -89,6 +91,12 @@ public class Executor extends Thread implements ModelObject { | |
*/ | ||
private volatile Queue.Executable executable; | ||
|
||
/** | ||
* Used to mark that the execution is continuing asynchronously even though {@link Executor} as {@link Thread} | ||
* has finished. | ||
*/ | ||
private AsynchronousExecution asynchronousExecution; | ||
|
||
/** | ||
* When {@link Queue} allocates a work for this executor, this field is set | ||
* and the executor is {@linkplain Thread#start() started}. | ||
|
@@ -124,27 +132,38 @@ public void interrupt() { | |
interrupt(Result.ABORTED); | ||
} | ||
|
||
void interruptForShutdown() { | ||
interrupt(Result.ABORTED, true); | ||
} | ||
/** | ||
* Interrupt the execution, | ||
* but instead of marking the build as aborted, mark it as specified result. | ||
* | ||
* @since 1.417 | ||
*/ | ||
public void interrupt(Result result) { | ||
interrupt(result, false); | ||
} | ||
|
||
private void interrupt(Result result, boolean forShutdown) { | ||
Authentication a = Jenkins.getAuthentication(); | ||
if (a == ACL.SYSTEM) | ||
interrupt(result, new CauseOfInterruption[0]); | ||
interrupt(result, forShutdown, new CauseOfInterruption[0]); | ||
else { | ||
// worth recording who did it | ||
// avoid using User.get() to avoid deadlock. | ||
interrupt(result, new UserInterruption(a.getName())); | ||
interrupt(result, forShutdown, new UserInterruption(a.getName())); | ||
} | ||
} | ||
|
||
/** | ||
* Interrupt the execution. Mark the cause and the status accordingly. | ||
*/ | ||
public void interrupt(Result result, CauseOfInterruption... causes) { | ||
interrupt(result, false, causes); | ||
} | ||
|
||
private void interrupt(Result result, boolean forShutdown, CauseOfInterruption... causes) { | ||
if (LOGGER.isLoggable(FINE)) | ||
LOGGER.log(FINE, String.format("%s is interrupted(%s): %s", getDisplayName(), result, Util.join(Arrays.asList(causes),",")), new InterruptedException()); | ||
|
||
|
@@ -163,7 +182,11 @@ public void interrupt(Result result, CauseOfInterruption... causes) { | |
this.causes.add(c); | ||
} | ||
} | ||
super.interrupt(); | ||
if (asynchronousExecution != null) { | ||
asynchronousExecution.interrupt(forShutdown); | ||
} else { | ||
super.interrupt(); | ||
} | ||
} | ||
|
||
public Result abortResult() { | ||
|
@@ -238,23 +261,14 @@ public void run() { | |
if (LOGGER.isLoggable(FINE)) | ||
LOGGER.log(FINE, getName()+" is now executing "+executable); | ||
queue.execute(executable, task); | ||
} catch (AsynchronousExecution x) { | ||
x.setExecutor(this); | ||
this.asynchronousExecution = x; | ||
} catch (Throwable e) { | ||
// for some reason the executor died. this is really | ||
// a bug in the code, but we don't want the executor to die, | ||
// so just leave some info and go on to build other things | ||
LOGGER.log(Level.SEVERE, "Executor threw an exception", e); | ||
workUnit.context.abort(e); | ||
problems = e; | ||
} finally { | ||
long time = System.currentTimeMillis()-startTime; | ||
if (LOGGER.isLoggable(FINE)) | ||
LOGGER.log(FINE, getName()+" completed "+executable+" in "+time+"ms"); | ||
try { | ||
workUnit.context.synchronizeEnd(executable,problems,time); | ||
} catch (InterruptedException e) { | ||
workUnit.context.abort(e); | ||
} finally { | ||
workUnit.setExecutor(null); | ||
if (asynchronousExecution == null) { | ||
finish1(problems); | ||
} | ||
} | ||
} catch (InterruptedException e) { | ||
|
@@ -267,12 +281,49 @@ public void run() { | |
causeOfDeath = e; | ||
LOGGER.log(SEVERE, "Unexpected executor death", e); | ||
} finally { | ||
if (causeOfDeath==null) | ||
// let this thread die and be replaced by a fresh unstarted instance | ||
owner.removeExecutor(this); | ||
if (asynchronousExecution == null) { | ||
finish2(); | ||
} | ||
} | ||
} | ||
|
||
private void finish1(@CheckForNull Throwable problems) { | ||
if (problems != null) { | ||
// for some reason the executor died. this is really | ||
// a bug in the code, but we don't want the executor to die, | ||
// so just leave some info and go on to build other things | ||
LOGGER.log(Level.SEVERE, "Executor threw an exception", problems); | ||
workUnit.context.abort(problems); | ||
} | ||
long time = System.currentTimeMillis() - startTime; | ||
LOGGER.log(FINE, "{0} completed {1} in {2}ms", new Object[] {getName(), executable, time}); | ||
try { | ||
workUnit.context.synchronizeEnd(this, executable, problems, time); | ||
} catch (InterruptedException e) { | ||
workUnit.context.abort(e); | ||
} finally { | ||
workUnit.setExecutor(null); | ||
} | ||
} | ||
|
||
queue.scheduleMaintenance(); | ||
private void finish2() { | ||
if (causeOfDeath == null) {// let this thread die and be replaced by a fresh unstarted instance | ||
owner.removeExecutor(this); | ||
} | ||
if (this instanceof OneOffExecutor) { | ||
owner.remove((OneOffExecutor) this); | ||
} | ||
queue.scheduleMaintenance(); | ||
} | ||
|
||
@Restricted(NoExternalUse.class) | ||
public void completedAsynchronous(@CheckForNull Throwable error) { | ||
try { | ||
finish1(error); | ||
} finally { | ||
finish2(); | ||
} | ||
asynchronousExecution = null; | ||
} | ||
|
||
/** | ||
|
@@ -357,14 +408,22 @@ public boolean isBusy() { | |
/** | ||
* Check if executor is ready to accept tasks. | ||
* This method becomes the critical one since 1.536, which introduces the | ||
* on-demand creation of executor threads. The entire logic should use | ||
* this method instead of {@link #isAlive()}, because it provides wrong | ||
* information for non-started threads. | ||
* on-demand creation of executor threads. Callers should use | ||
* this method instead of {@link #isAlive()}, which would be incorrect for | ||
* non-started threads or running {@link AsynchronousExecution}. | ||
* @return True if the executor is available for tasks | ||
* @since 1.536 | ||
*/ | ||
public boolean isActive() { | ||
return !started || isAlive(); | ||
return !started || asynchronousExecution != null || isAlive(); | ||
} | ||
|
||
/** | ||
* If currently running in asynchronous mode, returns that handle. | ||
* @since TODO | ||
*/ | ||
public @CheckForNull AsynchronousExecution getAsynchronousExecution() { | ||
return asynchronousExecution; | ||
} | ||
|
||
/** | ||
|
@@ -377,7 +436,7 @@ public boolean isParking() { | |
/** | ||
* If this thread dies unexpectedly, obtain the cause of the failure. | ||
* | ||
* @return null if the death is expected death or the thread is {@link #isAlive() still alive}. | ||
* @return null if the death is expected death or the thread {@link #isActive}. | ||
* @since 1.142 | ||
*/ | ||
public Throwable getCauseOfDeath() { | ||
|
@@ -529,7 +588,7 @@ public HttpResponse doStop() { | |
@RequirePOST | ||
public HttpResponse doYank() { | ||
Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER); | ||
if (isAlive()) | ||
if (isActive()) | ||
throw new Failure("Can't yank a live executor"); | ||
owner.removeExecutor(this); | ||
return HttpResponses.redirectViaContextPath("/"); | ||
|
@@ -593,6 +652,32 @@ protected Object call(Object o, Method m, Object[] args) throws Throwable { | |
return IMPERSONATION.get(); | ||
} | ||
|
||
/** | ||
* Finds the executor running a given process. | ||
* @param executable a possibly running executable | ||
* @return the executor (possibly a {@link OneOffExecutor}) whose {@link Executor#getCurrentExecutable} matches that, or null | ||
* @since TODO | ||
*/ | ||
public static Executor of(Executable executable) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
Jenkins jenkins = Jenkins.getInstance(); | ||
if (jenkins == null) { | ||
return null; | ||
} | ||
for (Computer computer : jenkins.getComputers()) { | ||
for (Executor executor : computer.getExecutors()) { | ||
if (executor.getCurrentExecutable() == executable) { | ||
return executor; | ||
} | ||
} | ||
for (Executor executor : computer.getOneOffExecutors()) { | ||
if (executor.getCurrentExecutable() == executable) { | ||
return executor; | ||
} | ||
} | ||
} | ||
return null; | ||
} | ||
|
||
/** | ||
* Returns the estimated duration for the executable. | ||
* Protects against {@link AbstractMethodError}s if the {@link Executable} implementation | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -88,6 +88,7 @@ public void execute(@Nonnull Runnable task, ResourceActivity activity ) throws I | |
try { | ||
task.run(); | ||
} finally { | ||
// TODO if AsynchronousExecution, do that later | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may need to revisit the locking after #1596 |
||
synchronized(this) { | ||
inProgress.remove(activity); | ||
inUse = ResourceList.union(resourceView); | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finds the executor that was running a given process while this method was being called. There is the potential that the process may complete by the time this method returns and thus a non-null result could correspond to an Executor running a different process
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think so. As of 1.536, an
Executor
is thrown out after each use. Thus at worst it will return null for something that was running earlier. But this deserves clarification.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well you can still have
Executor executable = Executor.of(executable); assert executable.getCurrentExecutable() == executable
give an Assertion error` as the executable can finish concurrent with returning from the method.