Release node body reference when finishing a PlaceholderTask

[basil]

Problem

After upgrading workflow-durable-task-step from 2.31 to 2.32 in workflow-cps-global-lib-plugin, LibraryMemoryTest#loaderReleased starts failing with the following stack trace:

[ERROR] loaderReleased(org.jenkinsci.plugins.workflow.libs.LibraryMemoryTest)  Time elapsed: 1.561 s  <<< ERROR!
java.lang.NullPointerException
	at org.netbeans.insane.live.Path.getField(Path.java:146)
	at org.netbeans.insane.live.Path.toString(Path.java:140)
	at org.netbeans.insane.live.Path.toString(Path.java:138)
	at java.lang.String.valueOf(String.java:2994)
	at java.lang.StringBuilder.append(StringBuilder.java:131)
	at java.util.AbstractMap.toString(AbstractMap.java:559)
	at java.lang.String.valueOf(String.java:2994)
	at java.lang.StringBuilder.append(StringBuilder.java:131)
	at org.jvnet.hudson.test.MemoryAssert.assertGC(MemoryAssert.java:187)
	at org.jenkinsci.plugins.workflow.libs.LibraryMemoryTest.loaderReleased(LibraryMemoryTest.java:85)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:48)
	at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:599)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:298)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:292)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.lang.Thread.run(Thread.java:748)

Evaluation

Bisection revealed that the problem does not occur with workflow-durable-task-step 2.31 (see jenkinsci/workflow-cps-global-lib-plugin#80) but does occur with workflow-durable-task-step 2.32 (see jenkinsci/workflow-cps-global-lib-plugin#81). Looking at the changes in the compare view, I noticed two new fields: DurableTaskStep#removedNodeDiscovered and PlaceholderTask#body, both added in #104. DurableTaskStep#removedNodeDiscovered is a long and is unlikely to be the leak, so I hypothesized that PlaceholderTask#body was the leak. Sure enough, when I changed body = context.newBodyInvoker().[...] back to context.newBodyInvoker().[...] in PlaceholderExecutable#run (the only place where PlaceholderExecutable#body was ever set) and re-ran LibraryMemoryTest#loaderReleased, it passed. This confirmed that the proximate source of the problem was PlaceholderTask#body.

While I don't know enough about this plugin to be completely sure, based on the above analysis I strongly suspect that the cause of the problem is that PlaceholderTask#body is being leaked, exposing the workflow-cps-global-lib-plugin test failure in LibraryMemoryTest#loaderReleased.

Solution

Assuming that the above evaluation is correct, the solution would be to release PlaceholderExecutable#body when it is no longer needed. Without knowing very much about this plugin, PlaceholderTask#finish seemed like as good a place as any to do this. Much to my surprise, workflow-cps-global-lib-plugin's LibraryMemoryTest#loaderReleased test started passing again once I made this change.

If I am misunderstanding the cause of the problem or if there is a better solution, please feel free to let me know.

[jglick]

Seems fine, though it would be easier to make body be a WeakReference<BodyExecution> I suppose.

[basil]

Seems fine, though it would be easier to make body be a WeakReference<BodyExecution> I suppose.

Sure, I reimplemented the change with body as a WeakReference<BodyExecution>. Feel free to let me know if this isn't what you had in mind.

[jglick]

Nice and simple. You checked that the WeakReference rewrite solves the test failure?

[basil]

You checked that the WeakReference rewrite solves the test failure?

Yes, of course. But don't take my word for it, see the results for yourself in jenkinsci/workflow-cps-global-lib-plugin#82.