2.0 proposal

96 views
Skip to first unread message

James Nord

unread,
Oct 19, 2015, 9:13:08 AM10/19/15
to Jenkins Developers
Hi all,

So we have strived to keep Jenkins backwards compatible and Kohsuke did want to try and keep backwards compatibility where possible.

But I have a proposal I would like to table that will break this as the current situation is not sustainable in my opinion.

All non-private fields (except public static finals) in the core code shall be removed (replaced with getters/setters) - and there shall not be any use of the bytecode-compatibility-transformer and @AdaptField.

Why?

  1. the use of fields breaks encapsulation and makes maintaining the code and enforcing correct locking harder (why we need to remove them)
  2. We are now in the realms where the bytecode compatability transformer is causing issues (it has been for a while but...) (why we need to stop using the bct)
    1. the rewriting now requires to load classes which they themselves may need to be re-written and is brittle (currently this just blows up but for JDK6+ classes this will become a much more prevelant issue)
    2. the transformer rewrites classes that do not need re-writing.  In order to do this correctly we need to traverse the entire class hierarchy due to the format of the bytecode - and this also is brittle (potentially loading classes whilst loading classes - which will cause further issues).
    3. I can see the above blowing up at some point with some funky circular reference which is perfectly legal

Infact having just fixed JENKINS-30820, we now see broken jruby runtime plugin with a ClassCircularityError.

https://issues.jenkins-ci.org/browse/JENKINS-19383  (not fixed  just hidden - still rewrites this class - hence the ticket below)

Post JDK6 the bytecode is now much more complex - to the point that until jdk 1.8 update 60 (just released) the jvm bytecode verifier was letting invalid bytecode past the verifier - which has suddenly showed some more broken transformations prior to JENKINS-28781 landing in 1.633 - and who knows what JDK9 will bring in terms of new op-codes or requirements.

/James

Stephen Connolly

unread,
Oct 19, 2015, 9:51:41 AM10/19/15
to jenkin...@googlegroups.com
+1000 (having had to dance around the @AdaptField on private fields
called "id" in Jenkins 1.602+ more times than I'd care to recall
already this year)
> --
> You received this message because you are subscribed to the Google Groups
> "Jenkins Developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to jenkinsci-de...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/jenkinsci-dev/54f53a29-4ce1-4582-830a-78c0c2274de3%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

Jesse Glick

unread,
Oct 19, 2015, 12:19:47 PM10/19/15
to Jenkins Dev
On Mon, Oct 19, 2015 at 9:13 AM, James Nord <jn...@cloudbees.com> wrote:
> there shall not be any use of the bytecode-compatibility-transformer and @AdaptField.

I see your point; this looks very scary, and we clearly do not have
any JVM experts on hand to design a system that is truly foolproof.
(Or sufficiently powerful—`@AdaptField` only helps with a very limited
set of problems, and if we want to continue splitting plugins from
core¹ we need a lot more.) On the other hand we are looking at the
potential for a *lot* of `LinkageError`s hitting innocent users during
an upgrade.

I wonder if we can find some more static, debuggable, easily
understood procedure for solving this class of problem. For example

1. Deprecate things we do not want people using and introduce
replacements. (As soon as possible—not waiting for 2.0.)
2. Provide a precise, perhaps mechanically enforceable refactoring.
NetBeans declarative refactorings² are better than nothing, though I
would like to see a batch-mode tool (+ Maven plugin) to apply them.
3. Enhance `plugin-compat-tester` to check for deprecation warnings
when compiling against the newest dependency, or otherwise find usages
of members we propose to delete. Have a team responsible for finding &
fixing failures, if necessary incrementing the plugin’s baseline
version to get access to the replacement.
4. Introduce a facility to the Jenkins plugin/update system allowing
us to declare that a specific version of the plugin is the first known
to be compatible with a specific version of a (core or plugin)
dependency. If you attempt to upgrade the dependency you will be
blocked unless you also agree to update the dependent.


¹https://issues.jenkins-ci.org/issues/?jql=resolution%20%3D%20Unresolved%20and%20labels%20in%20(split-plugins-from-core)
²Example: https://github.com/jenkinsci/jenkins/blob/3d2956d9ad65e34cc74949e5b397b7475c6cb04b/core/src/main/resources/META-INF/upgrade/AbstractTestResultAction.hint

oliver gondža

unread,
Oct 19, 2015, 2:04:53 PM10/19/15
to jenkin...@googlegroups.com
That all makes sense, though to implement that we need a policy to remove
deprecated API first. Removing non-private fields should be done by
deprecating it first.

I am tracking [1] for this effort. I would like to have a look at early
deprecated API use detection.

[1] https://issues.jenkins-ci.org/browse/JENKINS-31035

--
oliver

Kohsuke Kawaguchi

unread,
Oct 19, 2015, 2:19:09 PM10/19/15
to jenkin...@googlegroups.com
The part about proactively changing non-private fields is too reckless. It presumably breaks a significant number of plugins, and it does so for no good reasons. I say no good reasons because most of these fields need not be adopted ever.

IIUC, the pain that motivated you is the side effect of bytecode compatibility transformer, so what we should be thinking about is how to minimize the use & impact of BCT.

For example, Queue.Item.id has known fixed set of subtypes, so we can restrict the rewrite to a much smaller subset. And if we want to discourage the use of BCT, we can do that, too.


--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/54f53a29-4ce1-4582-830a-78c0c2274de3%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.



--
Kohsuke Kawaguchi

Jesse Glick

unread,
Oct 19, 2015, 3:19:44 PM10/19/15
to Jenkins Dev
On Mon, Oct 19, 2015 at 2:19 PM, Kohsuke Kawaguchi <k...@kohsuke.org> wrote:
> Queue.Item.id has known fixed set of subtypes, so we can restrict the rewrite to a much smaller subset.

Not sure I follow that. IIUC the problem is outside references to
`id`. The number of subtypes of `Item` (and the fact that they are all
in core) is irrelevant.

> if we want to discourage the use of BCT, we can do that, too.

I think to gain the benefits it has to be completely removed, not just
“discouraged”.

James Nord

unread,
Oct 19, 2015, 5:20:02 PM10/19/15
to Jenkins Developers


On Monday, October 19, 2015 at 9:19:44 PM UTC+2, Jesse Glick wrote:
On Mon, Oct 19, 2015 at 2:19 PM, Kohsuke Kawaguchi <k...@kohsuke.org> wrote:
> Queue.Item.id has known fixed set of subtypes, so we can restrict the rewrite to a much smaller subset.

Not sure I follow that. IIUC the problem is outside references to
`id`. The number of subtypes of `Item` (and the fact that they are all
in core) is irrelevant.

All we know is that class X has a field called `id`
In order to see if it is truly Item.id being referenced you need to load all the subclasses of X and walk them (we are back to loading subclassess in the classloader - eek)

Kohsuke Kawaguchi

unread,
Oct 19, 2015, 6:58:30 PM10/19/15
to jenkin...@googlegroups.com
2015-10-19 12:19 GMT-07:00 Jesse Glick <jgl...@cloudbees.com>:
On Mon, Oct 19, 2015 at 2:19 PM, Kohsuke Kawaguchi <k...@kohsuke.org> wrote:
> Queue.Item.id has known fixed set of subtypes, so we can restrict the rewrite to a much smaller subset.

Not sure I follow that. IIUC the problem is outside references to
`id`. The number of subtypes of `Item` (and the fact that they are all
in core) is irrelevant.

The only reference to 'id' that we need to rewrite is those for Queue.Item and its subtypes. Today BCT doesn't assume that the subtypes are finite fixed set, so it rewrites every reference to com.example.Foo#id based on the assumption that it might be a subtype of Queue.Item.

By knowing all the subtypes, we can drastically reduces the amount of rewrite.

--
Kohsuke Kawaguchi

James Nord

unread,
Oct 20, 2015, 5:53:07 AM10/20/15
to Jenkins Developers
We don't know the subtypes at compile time - nor at runtime whilst the classes are being loaded - without loading more classes (or at least loading the bytecode for those classes and inspecting said bytecode)

Also it's not the subtypes that need rewriting - it's the accessors of the field in other classes need to be re-written to use the new methods - (JENKINS-28799) - to reduce scope but it is just a sticky-plaster that reduces the amount of incorrect bytecode we may create.

Lets for argument sake say we did need to re-write a field in the ruby plugin - would/could we still not hit something like JENKINS-31019?

Kohsuke Kawaguchi

unread,
Oct 20, 2015, 2:38:52 PM10/20/15
to jenkin...@googlegroups.com
2015-10-20 2:53 GMT-07:00 James Nord <jn...@cloudbees.com>:


On Tuesday, October 20, 2015 at 12:58:30 AM UTC+2, Kohsuke Kawaguchi wrote:


2015-10-19 12:19 GMT-07:00 Jesse Glick <jgl...@cloudbees.com>:
On Mon, Oct 19, 2015 at 2:19 PM, Kohsuke Kawaguchi <k...@kohsuke.org> wrote:
> Queue.Item.id has known fixed set of subtypes, so we can restrict the rewrite to a much smaller subset.

Not sure I follow that. IIUC the problem is outside references to
`id`. The number of subtypes of `Item` (and the fact that they are all
in core) is irrelevant.

The only reference to 'id' that we need to rewrite is those for Queue.Item and its subtypes. Today BCT doesn't assume that the subtypes are finite fixed set, so it rewrites every reference to com.example.Foo#id based on the assumption that it might be a subtype of Queue.Item.

By knowing all the subtypes, we can drastically reduces the amount of rewrite.


We don't know the subtypes at compile time - nor at runtime whilst the classes are being loaded - without loading more classes (or at least loading the bytecode for those classes and inspecting said bytecode)

I was thinking about having a human statically listing them.

Also it's not the subtypes that need rewriting - it's the accessors of the field in other classes need to be re-written to use the new methods - (JENKINS-28799) - to reduce scope but it is just a sticky-plaster that reduces the amount of incorrect bytecode we may create.

I've heard this BCT issue described as "whack a mole", from which I assumed that the impact is proportional to the use of this feature, in which case reducing the amount of rewrite would help considerably.

But as I look more into it, it feels less and less like that. The picture I'm getting now is that JENKINS-28799 is about BCT not updating StackMapFrame properly, and our attempt to fix it introduced other problems like JENKINS-31019.

If those are the only issue, I still think the overall bytecode transformation strategy in BCT is sound. Given that stack map frame recomputation requires classloading, my suggestion is to tweak the transformation so that it doesn't require adjusting stack frame map. I think this is relatively easily achievable by moving the transformed code into a separate method so that bytecode index remain the same.

That is, whereas today the transformation is as follows:

    ... bunch of bytecode ...
    GETFIELD com.example.Foo#id
    ... bunch of bytecode ...

          => 

    ... bunch of bytecode ...
    if ( LHS instanceof TYPE ) {
         INVOKEVIRTUAL com.example.Foo#getId()
    } else {
         GETFIELD com.example.Foo#id
    }
    ... bunch of bytecode ...

Replace this with:

    ... bunch of bytecode ...
    INVOKSTATIC helperMethodWeAdd(Foo)
    ... bunch of bytecode ...

... where the helper method is the if/else block. The stack map analysis of the helper method does not contain any joinining of flow, so there's no need to compute the common base type, and hence it shouldn't require any classloading.

(Alternatively, I think it is also possible to just update existing stack frame map incrementally by inserting additional intermediate frames.)

--
Kohsuke Kawaguchi

Stephen Connolly

unread,
Oct 20, 2015, 3:10:03 PM10/20/15
to jenkin...@googlegroups.com
/me suspects somebody doesn't want to give up their golden hammer ;-)
> --
> You received this message because you are subscribed to the Google Groups
> "Jenkins Developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to jenkinsci-de...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/jenkinsci-dev/CAN4CQ4zidKNqzy-7HJhbEEj2A_jU1zK5k75hapCG-QDScQhHzA%40mail.gmail.com.

Jesse Glick

unread,
Oct 20, 2015, 5:40:55 PM10/20/15
to Jenkins Dev
On Tue, Oct 20, 2015 at 2:38 PM, Kohsuke Kawaguchi <k...@kohsuke.org> wrote:
> …this is relatively easily achievable by moving the transformed code into a
> separate method so that bytecode index remain the same.

I am not going to sleep more soundly tonight. :-)

I think the broader issue is that use of runtime bytecode manipulation
has proven to be subject to catastrophic bugs which are tricky to
understand, much less fix. And pity the developer who attaches a Java
debugger with a `*-sources.jar` and tries to step through rewritten
code. We should be considering simpler, lower-tech options. If a field
has been replaced with a getter, or whatever, let us make sure we fix
the deprecated source code references and release those fixes and make
sure users are not running the wrong version.

Slide

unread,
Oct 20, 2015, 5:42:10 PM10/20/15
to Jenkins Dev
+1

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

Artur Szostak

unread,
Oct 21, 2015, 5:19:00 AM10/21/15
to jenkin...@googlegroups.com
Guys, are you writing compilers / compiler tools or a continuous integration / deployment tool?
Fix the API's, refactor the source code, update the plugin code and recompile. Hop to it. ;-)
Don't use these kinds of hacks. It leads to the dark side.

Sorry if I sound harsh. But the things you are trying to do on a supposedly production ready system are insane. You are all making me more and more scared of upgrading or doing anything with my Jenkins system by the minute. :-(

Artur

Manuel Jesús Recena Soto

unread,
Oct 21, 2015, 7:27:27 AM10/21/15
to Jenkins Developers
I totally agree!
> --
> You received this message because you are subscribed to the Google Groups
> "Jenkins Developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to jenkinsci-de...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/jenkinsci-dev/3C350C9D1C9DEE47BAEA43102B433C9EBE5E1151%40HQMBX2.ads.eso.org.
>
> For more options, visit https://groups.google.com/d/optout.



--
Manuel Recena Soto
* manuelrecena.com [/blog]
* linkedin.com/in/recena

Kanstantsin Shautsou

unread,
Oct 21, 2015, 1:24:53 PM10/21/15
to Jenkins Developers, aszo...@partner.eso.org
That's what all people asking about last years, but there is number of old developers (not jenkins admins/users) that likes developing tricky code as it their main work.
KISS

Kohsuke Kawaguchi

unread,
Oct 21, 2015, 1:29:01 PM10/21/15
to jenkin...@googlegroups.com

I feel like there's a gap in perception here. I'm sensing that you are feeling that this is a systematic problem where fixing one problem at hand wouldn't solve problems down the road. That there will be more of these problems down the road.

In contrast, what I see is one bug (failing to update StackMapFrame) and two failed attempts to fix it, and we are this close to fixing it properly. And class file format change that drives issues like this only happens at major Java releases, and Java9 has no such changes currently in it. Hopefully this helps you see why I feel like we are throwing the baby out with the bathwater.

And it's not that problems are hard to understand or hard to fix. What makes this issue painful is that it blows up in unexpected places.

Anyway, I'm losing this argument.

Now, if you are going to remove BCT altogether, what do we do with existing uses of them? Queue.Item#id is a relatively recent change. Are we just going to break plugins that use them?

--
Kohsuke Kawaguchi

Jesse Glick

unread,
Oct 21, 2015, 3:22:57 PM10/21/15
to Jenkins Dev
On Wed, Oct 21, 2015 at 1:28 PM, Kohsuke Kawaguchi <k...@kohsuke.org> wrote:
> That there will be more of these problems down the road.

Perhaps. More important is the ongoing burden of not knowing whether
what you wrote in `src/main/java/**/*.java`, or what you loaded from
`*-sources.jar`, is actually what is running.

> I feel like we are throwing the baby out with the bathwater.

This baby is Otesánek. :-)

> it's not that problems are hard to understand or hard to fix.

For you perhaps.

> if you are going to remove BCT altogether, what do we do with existing
> uses of them? Queue.Item#id is a relatively recent change. Are we just going
> to break plugins that use them?

There was no fully developed proposal yet. My idea was to have an
automated system for detecting plugin source repos still using the
deprecated API, then fixing those, and concurrently blocking the old
plugin releases from being used. So a user would never see linkage
errors, but they might be required to upgrade a plugin when upgrading
core (or a lower-level plugin).
Reply all
Reply to author
Forward
0 new messages