Skip to content
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

[JENKINS-25890] Use own thread pool for CpsStepContext.getProgramPromise #68

Merged
merged 3 commits into from Mar 4, 2015

Conversation

@jglick
Copy link
Member

jglick commented Mar 3, 2015

JENKINS-25890

Amends fix in #65. That turned out to cause WorkflowTest.acquireWorkspace to fail mostly reliably—it was already @RandomlyFails—one reason why a PR from @jtnord to use the “flaky test” feature of Surefire would be appreciated.

The problem was that rehydration of pickles via TryRepeatedly depends on jenkins.util.Timer, which is a cached thread pool with 10 threads, yet the original fix added a task to this same pool for a couple of commonly used CpsStepContext methods. These tasks would block waiting for the program to load, which cannot happen until all the pickles are rehydrated. Of the twelve pickles loaded in this test (six per loaded build), ten got rehydrated fine, but the last two were ExecutorPickles, which are slow to rehydrate (need to wait for slaves to come online and the queue to be maintained). The first try failed due to offline slaves; tryLater was called, but the task was not scheduled because at that point all ten Timer threads were busy. Thus a deadlock, possibly reproducible only when multiple builds were being loaded.

Use of Timer from TryRepeatedly seems acceptable in general since each “try” is intended to be nonblocking. It was the new use from CpsStepContext.getProgramPromise that introduced the problem. Using Executors.newCachedThreadPool seems more appropriate here, at least until some future refactoring allows WorkflowRun.Owner to produce a proper ListenableFuture<FlowExecution> so that we can operate on callbacks rather than blocking a bunch of threads during startup.

@reviewbybees esp. @kohsuke

@tfennelly

This comment has been minimized.

Copy link
Member

tfennelly commented Mar 3, 2015

👍

@@ -224,11 +227,12 @@ public String getDisplayName() {
throw new IOException(e);
}
}


private static final ExecutorService getProgramPromiseExecutorService = Executors.newCachedThreadPool(new NamingThreadFactory(new DaemonThreadFactory(), "CpsStepContext.getProgramPromise"));

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 3, 2015

Member

Isn't this another thread pool that will need fix-up for testing?

This comment has been minimized.

Copy link
@jglick

jglick Mar 3, 2015

Author Member

What do you mean by “fix-up for testing”?

This comment has been minimized.

Copy link
@jglick

jglick Mar 3, 2015

Author Member

Remember this is only used to implement a ListenableFuture; every caller should just be calling .get() on that immediately, or checking .isDone(), or perhaps waiting on the result some other way.

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 3, 2015

Member

So it doesn't need to be context resetting then?

This comment has been minimized.

Copy link
@jglick

jglick Mar 3, 2015

Author Member

ContextResettingExecutorService? I do not think it matters (and Timer is not, currently), but I suppose it cannot hurt.

@jglick jglick changed the title [JENKINS-25890] Use own thread pool for CpsStepContext.getProgramPromise [WiP] [JENKINS-25890] Use own thread pool for CpsStepContext.getProgramPromise Mar 3, 2015
@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Mar 3, 2015

Withdrawing this from consideration for a while, since I realized that isReady is probably going to usually be false as of #65—which is not blocking, but certainly is not useful either. I think the future needs to be cached.

@jglick jglick changed the title [WiP] [JENKINS-25890] Use own thread pool for CpsStepContext.getProgramPromise [JENKINS-25890] Use own thread pool for CpsStepContext.getProgramPromise Mar 3, 2015
@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Mar 3, 2015

Fixed that issue, back to @reviewbybees.

@stephenc

This comment has been minimized.

Copy link

stephenc commented on 6a3593b Mar 3, 2015

jglick added a commit that referenced this pull request Mar 4, 2015
[JENKINS-25890] Use own thread pool for CpsStepContext.getProgramPromise
@jglick jglick merged commit 729984d into jenkinsci:master Mar 4, 2015
1 check passed
1 check passed
Jenkins This pull request looks good
Details
@jglick jglick deleted the jglick:starvation-JENKINS-25890 branch Mar 4, 2015
jglick added a commit to jglick/pipeline-plugin that referenced this pull request Jan 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.