Skip to content

No messages printed from block-scoped steps after they invoked their bodies#19

Merged
jglick merged 1 commit intojenkinsci:masterfrom
jglick:LogActionImpl-body
Oct 19, 2016
Merged

No messages printed from block-scoped steps after they invoked their bodies#19
jglick merged 1 commit intojenkinsci:masterfrom
jglick:LogActionImpl-body

Conversation

@jglick
Copy link
Member

@jglick jglick commented Oct 18, 2016

Superseded by #15 but this is a far more conservative and localized change.

Background: in JENKINS-34637 I would like TimeoutStepExecution to print something to the build log when its body times out. Without this patch, that is impossible—the log message is discarded.

@reviewbybees

@ghost
Copy link

ghost commented Oct 18, 2016

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.

@stephenc
Copy link
Member

🐝 as I understand it... but my comprehension is not at its best right nie

@jglick jglick merged commit 22bf59b into jenkinsci:master Oct 19, 2016
@jglick jglick deleted the LogActionImpl-body branch October 19, 2016 15:12
* (for example by calling {@link StepContext#get} on demand rather than by using {@link StepContextParameter}).
*/
private static boolean isRunning(FlowNode node) {
if (node instanceof BlockStartNode) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jglick I think this is really iffy -- you are potentially scanning the flow back to the beginning of time if it happens to be a BlockStartNode.

I would caution against this because it is likely to lead to O(n^2) performance if invoked incautiously.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are potentially scanning the flow back to the beginning of time

Right, because in

node { // #1
  for (int i = 0; i < 1000; i++) {sh 'echo' /* #2 */}
}

the current head would include point 2 and we need to scan back to point 1 if this method is called. Now we could add caching etc. if there an API for it.

@jglick
Copy link
Member Author

jglick commented Oct 20, 2016

Seems to have caused a regression in EnvStepRunTest.parallel; under investigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants