Migrate from readResolve to unmarshal

[car-roll]

If a build is loaded from a saved state, and the build existed before #519 (i.e. v2686.v7c37e0578401), It is possible for the timings field to be NULL. This causes an NPE when the Timing is closed:

workflow-cps-plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java

Line 432 in 3f09155

timings.merge(kind.name(), System.nanoTime() - start, Long::sum);

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests - that demonstrates feature works or fixes the issue

[car-roll]

I basically just put back the null check that was removed in #519

[jglick]

Do we still need this addition?

[car-roll]

yikes, removed in #529

[Vlatombe]

I wonder if this is not a symptom of inconsistent (or at least, surprising) deserialization, where unmarshal is called but not readResolve. Should the current unmarshal method be patched to call readResolve at the end?

[dwnusbaum]

I wonder if this is not a symptom of inconsistent (or at least, surprising) deserialization, where unmarshal is called but not readResolve.

I tested this really quickly by adding logger calls in each method, and it does look like only unmarshal gets called during deserialization.

Should the current unmarshal method be patched to call readResolve at the end?

Maybe? I took a quick look through xstream to see if there was any discussion or documentation about combining custom converters with readResolve, but I did not notice anything. I could also see an argument for moving the logic in readResolve into unmarshal and deleting readResolve, at least as long as that doesn't cause any other issues.

[jglick]

moving the logic in readResolve into unmarshal and deleting readResolve

That sounds appropriate.

[car-roll]

When I move the code to

workflow-cps-plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java

Line 1715 in 3f09155

} else if (nodeName.equals("timings")) {

timings is still null.

Looking at the stack trace, I realize it is only calling WorkflowRun.reload(): https://github.com/jenkinsci/workflow-job-plugin/blob/04c4e75dce438ba5af785fbf3a7711cacd54f41e/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java#L538
So it is completely bypassing CpsFlowExecution.unmarshal() and just calling hudson.XmlFile.unmarshal via WorkflowJob.getLastBuild()

[dwnusbaum]

@car-roll I think you would need to inline readResolve later on (e.g. right before the return statement), not add the logic into the else if block, because if timings is missing from the XML file (i.e. if it is null), that block never gets entered.

[car-roll]

@dwnusbaum thank you for pointing out that glaring error 🤦
All is well now!

[dwnusbaum]

Might be worth adding a quick comment to ConverterImpl:

// Note: XStream ignores readResolve and writeReplace methods on types with custom Converter implementations, so use marshal and unmarshal instead.

[jglick]

@car-roll #528 (comment)