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-42556] Run more system threads as SYSTEM #2792

Merged
merged 2 commits into from Mar 14, 2017

Conversation

4 participants
@jglick
Copy link
Member

commented Mar 9, 2017

Suffices to fix JENKINS-42556 (and probably a variety of similar bugs, reported or not) by ensuring that code in Timer and Queue threads runs as SYSTEM, the way other background threads (for example, periodic tasks, or Jenkins startup) already were.

Proposed changelog entry: Run more system threads with full, rather than anonymous, authentication, to prevent bugs typically relating to jobs hidden from anonymous users.

@reviewbybees

@reviewbybees

This comment has been minimized.

Copy link

commented Mar 9, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

}

@Override
protected Runnable wrap(final Runnable r) {

This comment has been minimized.

Copy link
@jglick

jglick Mar 9, 2017

Author Member

Multiple inheritance would be useful here.

new DaemonThreadFactory(),
String.format("AtmostOneTaskExecutor[%s]", task)
)),
)), ACL.SYSTEM),

This comment has been minimized.

Copy link
@jglick

jglick Mar 9, 2017

Author Member

This fixes Queue.maintain to run as SYSTEM. Without that, Queue.Task.getFullDisplayName() is called when logging is enabled (and, as in #2791, even when it is not!) as anonymous, which can throw AccessDeniedException straight out of maintain¹, and…

¹Causing the queue to remove a PendingItem without ever adding the corresponding BlockedItem, so that getItem(long) suddenly and inexplicably starts returning null, breaking ExecutorPickle.

@@ -100,6 +107,7 @@ public Void call() throws Exception {
try {
inprogress.set(task.call());
} catch (Throwable t) {
LOGGER.log(Level.WARNING, null, t);

This comment has been minimized.

Copy link
@jglick

jglick Mar 9, 2017

Author Member

…that exception was being completely swallowed…

@@ -100,6 +107,7 @@ public Void call() throws Exception {
try {
inprogress.set(task.call());
} catch (Throwable t) {
LOGGER.log(Level.WARNING, null, t);
inprogress.setException(t);

This comment has been minimized.

Copy link
@jglick

jglick Mar 9, 2017

Author Member

…because this has no effect whatsoever. I do not really understand what @kohsuke was trying to accomplish in 666f0b4, but the inprogress field is only ever modified or checked for nullity—meaning it could just as well have been a boolean, as far as I can tell.

@@ -39,7 +41,8 @@ public static synchronized ScheduledExecutorService get() {
if (executorService == null) {
// corePoolSize is set to 10, but will only be created if needed.
// ScheduledThreadPoolExecutor "acts as a fixed-sized pool using corePoolSize threads"
executorService = new ErrorLoggingScheduledThreadPoolExecutor(10, new NamingThreadFactory(new DaemonThreadFactory(), "jenkins.util.Timer"));
// TODO consider also wrapping in ContextResettingExecutorService
executorService = new ImpersonatingScheduledExecutorService(new ErrorLoggingScheduledThreadPoolExecutor(10, new NamingThreadFactory(new DaemonThreadFactory(), "jenkins.util.Timer")), ACL.SYSTEM);

This comment has been minimized.

Copy link
@jglick

jglick Mar 9, 2017

Author Member

This fixes all sorts of background tasks to run as SYSTEM, the way you probably thought they already were, but apparently were not. In particular, TryRepeatedly, used to restore the state of a running Pipeline build after restart.

jglick referenced this pull request Mar 9, 2017

Fixing a bug.
pending was null as soon as task started running, so it was possible that another thread submits the task and have that executed while the first one was still running
* @param base the base service
* @param authentication for example {@link ACL#SYSTEM}
*/
public ImpersonatingExecutorService(ExecutorService base, Authentication authentication) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 10, 2017

Member

Maybe submit() operations should be performed with authentication as well

This comment has been minimized.

Copy link
@jglick

jglick Mar 14, 2017

Author Member

Not following. All submit overloads should be delegating to wrap.

@rsandell
Copy link
Member

left a comment

🐝

@oleg-nenashev
Copy link
Member

left a comment

Generally it looks good to me after googling for API usages in jenkinsci 🐝

import java.util.concurrent.TimeUnit;

/**
* Generalization of {@link InterceptingExecutorService} tp scheduled services.

This comment has been minimized.

Copy link
@oleg-nenashev
@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Mar 11, 2017

@jglick jglick merged commit dd0d578 into jenkinsci:master Mar 14, 2017

2 checks passed

Jenkins This pull request looks good
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@jglick jglick deleted the jglick:SYSTEM-JENKINS-42556 branch Mar 14, 2017

jglick added a commit that referenced this pull request Mar 14, 2017

@jglick jglick referenced this pull request Dec 8, 2017

Merged

Fixing a ClassCastException #123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.