JENKINS-48820# Fix for stack overflow in flownode sse event generation #1667
Conversation
|
@svanoort I think I am picking up right API to get immediate parent node. PTAL. |
| @@ -203,6 +204,9 @@ public void setBlockErrorAction(ErrorAction blockErrorAction) { | |||
| * Filters by class to mimic Item.getActions(class). | |||
| */ | |||
| public <T extends Action> Collection<T> getPipelineActions(Class<T> clazz) { | |||
| if(pipelineActions == null){ | |||
There was a problem hiding this comment.
Fixes NPE happening in cases where there are no actions added.
vivek
force-pushed
the
bug/JENKINS-48820-stack-overflow
branch
from
6910306
to
e7e49af
Compare
| } | ||
|
|
||
| private String toPath(List<String> branch) { | ||
| private String toPath(Iterator<String> branch) { |
There was a problem hiding this comment.
🐜 Seems like we could just pass an Iterable rather than a one-shot iterator?
| @@ -164,7 +149,7 @@ private static Message newMessage(PipelineEventChannel.Event event, FlowExecutio | |||
| return message; | |||
| } | |||
|
|
|||
| private Message newMessage(PipelineEventChannel.Event event, FlowNode flowNode, List<String> branch) { | |||
| private Message newMessage(PipelineEventChannel.Event event, FlowNode flowNode, Iterator<String> branch) { | |||
There was a problem hiding this comment.
🐜 Again, perhaps we can do an Iterable or collection rather than an Iterator?
| if (parent instanceof StepStartNode) { | ||
| if (parent.getAction(BodyInvocationAction.class) != null) { | ||
| return parent; | ||
| while (parentBlockId != null) { |
There was a problem hiding this comment.
I think what you want is myFlowNode.getEnclosingBlocks() which returns a list of blocks -- perhaps combined with Guava or Java 8 stream/filter APIs, perhaps via new NodeStepTypePredicate('parallel')) or FlowScanningUtils.hasActionPredicate(SomeAction.class)) -- it'll do all these goodies for you.
There was a problem hiding this comment.
@svanoort I looked at myFlowNode.getEnclosingBlocks(), it doesn't return blocks in order. This code was not written to me and I want to keep changes to minimum as it might break UI. Previous code was making sure preserving certain order, so I would prefer leaving the code with fixing SOF problem with minor enhancements.
There was a problem hiding this comment.
@vivek Could we just do a Lists.reverse, to maintain the order? It's iterating in inside-out order here so I think you want to simply invert that.
There was a problem hiding this comment.
@svanoort Right. I was not planning to use was due to this javadoc of GraphLookupView.findAllEnclosingBlockStarts() , https://github.com/jenkinsci/workflow-api-plugin/blob/a20f21fddc6831f51c3ceeefd3ed1b744b899e98/src/main/java/org/jenkinsci/plugins/workflow/graph/GraphLookupView.java#L55 that says order is not guaranteed. After clarification offline with you, I am fine using it. Please open a PR to fix the Javadoc to make it clear that inside-out order will be maintained.
|
Was this SO due to an infinite loop, or just by a complicated pipeline? |
|
@sophistifunk caused by large number of steps in pipelines about 4600+ as reported in https://issues.jenkins-ci.org/browse/JENKINS-48820. |
|
@svanoort PTAL |
| return null; | ||
| /* package: so that we can unit test it */ List<String> getBranch(FlowNode flowNode) { | ||
| return Lists.reverse(flowNode.getEnclosingBlocks().stream() | ||
| .map(FlowNode::getId).collect(Collectors.toList())); |
There was a problem hiding this comment.
Oh hey, if you just wanted the IDs there's node.getAllEnclosingIds()
Please don't kill me :) I've got APIs to do pretty much all the things you'd want to do with FlowNodes at this point...
There was a problem hiding this comment.
Ha ha. This is great. Let me make this change.
| StringBuilder builder = new StringBuilder(); | ||
| for (String leaf : branch) { | ||
| for(String leaf: branch){ |
There was a problem hiding this comment.
🐜 Can this path creation with just branch.stream().collect(Collectors.joining("/")) if you're feeling all Streams-y :)
There was a problem hiding this comment.
Right, there are many places in the code we need to change to adopt java 8, keeping the scope limited to just this fix, so be making such changes some other time.
Description
Fix for stack overflow error during pipeline execution SSE event generation, has other minor improvements.
See JENKINS-48820.
Submitter checklist
Reviewer checklist