Skip to content

Commit

Permalink
Merge pull request #38 from jglick/FlowNode.isActive-JENKINS-38223
Browse files Browse the repository at this point in the history 
[JENKINS-38223] Using FlowNode.isActive to improve JENKINS-45553
  • Loading branch information
@svanoort
svanoort committed Aug 22, 2017
2 parents 01f9538 + dec957a commit 236f06c
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 68 deletions.
25 changes: 9 additions & 16 deletions pom.xml
View file
Expand Up @@ -77,18 +77,17 @@
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-api</artifactId>
<version>2.15</version>
<exclusions>
<exclusion>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>scm-api</artifactId>
</exclusion>
</exclusions>
<version>2.21-20170818.152216-3</version> <!-- TODO https://github.com/jenkinsci/workflow-api-plugin/pull/45 -->
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>scm-api</artifactId>
<version>2.0.8</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>script-security</artifactId>
<version>1.26</version>
<version>1.27</version>
</dependency>
<dependency>
<groupId>org.jboss.marshalling</groupId>
Expand Down Expand Up @@ -117,20 +116,14 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>structs</artifactId>
<version>1.6</version>
<version>1.10</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-cps</artifactId>
<version>2.28</version>
<version>2.33</version>
<scope>test</scope>
<exclusions>
<exclusion>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>scm-api</artifactId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
Expand Down
41 changes: 6 additions & 35 deletions src/main/java/org/jenkinsci/plugins/workflow/support/actions/LogActionImpl.java
View file
Expand Up @@ -25,7 +25,6 @@
package org.jenkinsci.plugins.workflow.support.actions;

import com.google.common.base.Charsets;
import com.google.common.base.Predicates;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.console.AnnotatedLargeText;
import hudson.console.ConsoleLogFilter;
Expand All @@ -39,7 +38,6 @@
import java.io.PrintStream;
import java.io.UnsupportedEncodingException;
import java.nio.charset.Charset;
import java.util.List;
import java.util.concurrent.atomic.AtomicReference;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand All @@ -50,14 +48,7 @@
import org.jenkinsci.plugins.workflow.actions.LogAction;
import org.jenkinsci.plugins.workflow.flow.FlowExecutionOwner;
import org.jenkinsci.plugins.workflow.flow.GraphListener;
import org.jenkinsci.plugins.workflow.graph.BlockStartNode;
import org.jenkinsci.plugins.workflow.graph.FlowNode;
import org.jenkinsci.plugins.workflow.graphanalysis.AbstractFlowScanner;
import org.jenkinsci.plugins.workflow.graphanalysis.DepthFirstScanner;
import org.jenkinsci.plugins.workflow.graphanalysis.LinearBlockHoppingScanner;
import org.jenkinsci.plugins.workflow.graphanalysis.LinearScanner;
import org.jenkinsci.plugins.workflow.steps.StepContext;
import org.jenkinsci.plugins.workflow.steps.StepContextParameter;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.DoNotUse;
import org.kohsuke.stapler.framework.io.ByteBuffer;
Expand Down Expand Up @@ -94,7 +85,7 @@ public class LogActionImpl extends LogAction implements FlowNodeAction {
LOGGER.log(Level.FINE, "opened log for {0}", node.getDisplayFunctionName());
graphListener.set(new GraphListener.Synchronous() {
@Override public void onNewHead(FlowNode newNode) {
if (!isRunning(node)) {
if (!node.isActive()) {
node.getExecution().removeListener(graphListener.get());
result.getLogger().close();
LOGGER.log(Level.FINE, "closed log for {0}", node.getDisplayFunctionName());
Expand All @@ -109,9 +100,9 @@ public class LogActionImpl extends LogAction implements FlowNodeAction {
private transient volatile File log;
private String charset;

private LogActionImpl(FlowNode parent, Charset charset) {
if (!isRunning(parent)) {
throw new IllegalStateException("cannot start writing logs to a finished node " + parent);
private LogActionImpl(FlowNode parent, Charset charset) throws IOException {
if (!parent.isActive()) {
throw new IOException("cannot start writing logs to a finished node " + parent + " " + parent.getDisplayFunctionName() + " in " + parent.getExecution());
}
this.parent = parent;
this.charset = charset.name();
Expand All @@ -127,9 +118,9 @@ public AnnotatedLargeText<? extends FlowNode> getLogText() {
try {
getLogFile();
if (!log.exists()) {
return new AnnotatedLargeText<>(new ByteBuffer(), getCharset(), !isRunning(parent), parent);
return new AnnotatedLargeText<>(new ByteBuffer(), getCharset(), !parent.isActive(), parent);
}
return new AnnotatedLargeText<>(log, getCharset(), !isRunning(parent), parent);
return new AnnotatedLargeText<>(log, getCharset(), !parent.isActive(), parent);
} catch (IOException e) {
ByteBuffer buf = new ByteBuffer();
PrintStream ps;
Expand All @@ -145,26 +136,6 @@ public AnnotatedLargeText<? extends FlowNode> getLogText() {
}
}

/**
* Unlike {@link FlowNode#isRunning}, handles {@link BlockStartNode}s.
* Note that {@code WorkflowRun.copyLogs} will still currently refuse to consider steps
* which fail to obtain a {@link TaskListener} prior to invoking their body
* (for example by calling {@link StepContext#get} on demand rather than by using {@link StepContextParameter}).
*/
private static boolean isRunning(FlowNode node) {
if (node.isRunning()) {
return true;
}
if (node instanceof BlockStartNode) {
// Block start is considered running if currently executing nodes are part of the block
List<FlowNode> headNodes = node.getExecution().getCurrentHeads();
AbstractFlowScanner scanner = (headNodes.size() > 1) ? new DepthFirstScanner() : new LinearBlockHoppingScanner();
return scanner.findFirstMatch(headNodes, Predicates.equalTo(node)) != null;
} else {
return false;
}
}

/**
* The actual log file.
*/
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/jenkinsci/plugins/workflow/support/visualization/table/FlowGraphTable.java
View file
Expand Up @@ -260,7 +260,7 @@ private Row(FlowNode node) {
if (act != null) {
this.startTimeMillis = act.getStartTime();
this.hasStartTime = true;
if (node.isRunning()) {
if (node.isActive()) {
this.durationMillis=System.currentTimeMillis()-this.startTimeMillis;
this.hasTiming = true;
}
Expand Down
36 changes: 20 additions & 16 deletions src/test/java/org/jenkinsci/plugins/workflow/support/actions/LogActionImplTest.java
View file
Expand Up @@ -24,18 +24,19 @@

package org.jenkinsci.plugins.workflow.support.actions;

import com.google.inject.Inject;
import hudson.EnvVars;
import hudson.model.TaskListener;
import java.util.Collections;
import java.util.LinkedList;
import java.util.Set;
import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
import org.jenkinsci.plugins.workflow.job.WorkflowRun;
import org.jenkinsci.plugins.workflow.steps.AbstractStepDescriptorImpl;
import org.jenkinsci.plugins.workflow.steps.AbstractStepExecutionImpl;
import org.jenkinsci.plugins.workflow.steps.AbstractStepImpl;
import org.jenkinsci.plugins.workflow.steps.BodyExecutionCallback;
import org.jenkinsci.plugins.workflow.steps.Step;
import org.jenkinsci.plugins.workflow.steps.StepContext;
import org.jenkinsci.plugins.workflow.steps.StepDescriptor;
import org.jenkinsci.plugins.workflow.steps.StepExecution;
import org.junit.ClassRule;
import org.junit.Test;
import org.junit.Rule;
Expand All @@ -58,27 +59,30 @@ public class LogActionImplTest {
r.assertLogContains("atom step in A with 1 commands to go", b);
r.assertLogContains("logging from LBBL with 0 commands to go", b);
r.assertLogContains("atom step in B with 1 commands to go", b);
/* TODO misuse of FlowNode.isRunning in WorkflowRun.copyLogs prevents this from appearing unless we access TaskListener from start(); should be fixed by JENKINS-38381:
r.assertLogContains("logging from BL with 0 commands to go", b);
*/
}
public static class ChattyStep extends AbstractStepImpl {
public static class ChattyStep extends Step {
public final String pattern;
@DataBoundConstructor public ChattyStep(String pattern) {this.pattern = pattern;}
@TestExtension("logsAndBlocks") public static class DescriptorImpl extends AbstractStepDescriptorImpl {
public DescriptorImpl() {
super(Execution.class);
}
@Override public StepExecution start(StepContext context) throws Exception {
return new Execution(context, pattern);
}
@TestExtension("logsAndBlocks") public static class DescriptorImpl extends StepDescriptor {
@Override public String getFunctionName() {return "chatty";}
@Override public boolean takesImplicitBlockArgument() {return true;}
@Override public Set<? extends Class<?>> getRequiredContext() {
return Collections.singleton(TaskListener.class);
}
}
public static class Execution extends AbstractStepExecutionImpl {
@Inject(optional=true) transient ChattyStep step;
String pattern;
private static class Execution extends StepExecution {
Execution(StepContext context, String pattern) {
super(context);
this.pattern = pattern;
}
final String pattern;
LinkedList<Boolean> commands; // L ~ false to log, B ~ true to run block
@Override public boolean start() throws Exception {
commands = new LinkedList<>();
pattern = step.pattern;
for (char c : pattern.toCharArray()) {
if (c == 'L') {
commands.add(false);
Expand All @@ -102,7 +106,7 @@ private void run() throws Exception {
}
}
@Override public void stop(Throwable cause) throws Exception {}
private final class Callback extends BodyExecutionCallback {
private final class Callback extends BodyExecutionCallback { // not using TailCall since run() sometimes calls onSuccess itself
@Override public void onSuccess(StepContext context, Object result) {
try {
run();
Expand Down

0 comments on commit 236f06c

Please sign in to comment.