Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIXED JENKINS-48209] Prevent serialization of when expression closure #223

Merged
merged 3 commits into from Nov 27, 2017

Conversation

Projects
None yet
4 participants
@abayer
Copy link
Member

commented Nov 24, 2017

  • JENKINS issue(s):
  • Description:
    • This shouldn't really matter, in truth - I believe we actually regenerate the whole Root object from scratch on resume, though I could be misremembering. Regardless, even having the when expression closure be transient, the durability test (with when expressions added) still works right.
    • As to why we need to do this in the first place? Because something weird could be out there trying to some non-standard serialization of Describables. Ok, spoiler, there definitely is such a thing, and it does a straight XStream serialization of every Describable. But we use Describables as more than just UI config stuff - we use them for syntax validation and such for our internal extension points, like when conditions. And if that XStream serialization of a when expression with a sh step kicks in, well, all hell breaks loose. This works around that.
  • Documentation changes:
    • n/a
  • Users/aliases to notify:
[FIXED JENKINS-48209] Prevent serialization of when expression closure
This shouldn't really matter, in truth - I believe we actually
regenerate the whole Root object from scratch on resume, though I
could be misremembering. Regardless, even having the when expression
closure be transient, the durability test (with when expressions
added) still works right.

As to why we need to do this in the first place? Because something
weird could be out there trying to some non-standard serialization of
Describables. Ok, spoiler, there definitely is such a thing, and it
does a straight XStream serialization of every Describable. But we use
Describables as more than just UI config stuff - we use them for
syntax validation and such for our internal extension points, like
when conditions. And if that XStream serialization of a when
expression with a sh step kicks in, well, all hell breaks loose. This
works around that.

@abayer abayer added this to the 1.2.6 milestone Nov 24, 2017

@abayer abayer requested review from jglick, rsandell and svanoort Nov 24, 2017

@abayer

This comment has been minimized.

Copy link
Member Author

commented Nov 24, 2017

fwiw, the problematic pickler is in CloudBees Pipeline Templates, a proprietary plugin.

@reviewbybees

This comment has been minimized.

Copy link

commented Nov 24, 2017

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.

@rsandell
Copy link
Member

left a comment

🐝

@@ -386,4 +398,14 @@ protected boolean shouldExamineAllBuilds(@Nonnull SCMHead head) {
return false;
}
}

@TestExtension

This comment has been minimized.

Copy link
@rsandell

rsandell Nov 27, 2017

Member

shouldn't you isolate this to only be active for whenExprDurableTask?

This comment has been minimized.

Copy link
@abayer

abayer Nov 27, 2017

Author Member

Theoretically, we could do that, but I'm frankly fine with this being run on all the tests in here anyway.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2017

Hrm, can't reproduce the test failure locally. Fun.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2017

Oh. 'cos I'm not on Windows. Sigh.

@rsandell
Copy link
Member

left a comment

🐝

@abayer abayer merged commit 147204a into jenkinsci:master Nov 27, 2017

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.