[JENKINS-47517] Revert the change to the default behavior of Queue.Task.getCauseOfBlockage #3099

jglick · 2017-10-19T02:09:46Z

See JENKINS-47517. Reverts an inessential-seeming, and extremely incompatible, part of #3038.

Proposed changelog entries

StackOverflowError thrown under some conditions when using Pipeline on 2.85.

@reviewbybees @jenkinsci/code-reviewers

…sk.getCauseOfBlockage.

ghost · 2017-10-19T02:13:08Z

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.

svanoort

That's subtle... and nasty. Good catch.

Jimilian · 2017-10-19T10:22:29Z

@jglick any suggestion how to avoid such bugs in future? maybe there is some way to grep through all related plugins?

jglick · 2017-10-19T13:28:23Z

@oleg-nenashev asked if Util.isOverridden could be used to retain the fallback only when it is safe. We use this trick a lot to avoid stack overflows when upgrading APIs, but I did not see a clear way to use it in this case, because we want to know not just whether isBuildBlocked is overridden (it must be), but whether it delegates to getCauseOfBlockage. Suggestions welcome; in the meantime, it seemed that this fallback was not actually being used by the original patch and could be removed.

Another solution is of course to patch AfterRestartTask to take advantage of new default methods, and just return false unconditionally from isBuildBlocked. We probably want to do that anyway, but we cannot assume people will have upgraded workflow-job.

any suggestion how to avoid such bugs in future?

Ultimately there should be some process running plugin-compat-tester and acceptance-test-harness against jenkinsci/jenkins@master at least just prior to cutting a weekly release, and perhaps more often than that. I am not sure if that would have caught this or not—I noticed the StackOverflowError by accident in pipeline-stage-step test output but it did not cause a test failure. (The error was in a background thread incidental to the behavior being tested, and plugin test cases generally do not do anything like StartupTest.noWarnings.)

maybe there is some way to grep through all related plugins?

It is definitely possible to find usages of an API whose behavior you are proposing to change, deprecate, etc., as in this example. Somewhere (where?) there is also an index of core APIs using actual code analysis, rather than plain-text search. We used to have Sourcerer running on Jenkins but this has been broken for a long time.

jglick · 2017-10-19T13:33:53Z

@oleg-nenashev also asks whether we could have the equivalent of plugin-compat-tester run as part of jenkinsci/jenkins/Jenkinsfile (and thus even during core PR builds) for at least some “smoke” tests in selected plugins which are both important (~ in Setup Wizard recommended list, and with lots of active users); and likely to be broken by subtle core changes like this. Perhaps. plugin-compat-tester might be able to do it with some hackery; or jenkinsci/maven-hpi-plugin#66 might be a more maintainable path forward.

daniel-beck · 2017-10-19T13:38:49Z

It is definitely possible to find usages of an API whose behavior you are proposing to change, deprecate, etc., as in this example. Somewhere (where?) there is also an index of core APIs using actual code analysis, rather than plain-text search.

FWIW I usually adapt https://github.com/jenkins-infra/deprecated-usage-in-plugins to find what I'm looking for, e.g. jenkins-infra/usage-in-plugins#7. Signatures can be hard-coded in DeprecatedApi, e.g. methods.add("hudson/model/Queue#clearLeftItems") (which I was looking for in a recent investigation about a reported bug).

stephenc · 2017-10-19T14:02:41Z

if Util.isOverridden could be used to retain the fallback only when it is safe.

So the only way I can see to do that would be to use a ThreadLocal<?>

Get the current value of the ThreadLocal
If the current value != null, do not invoke
If the current value == null, set to this, invoke and finally clear again

We could go even more fancy to see if there are nested calls... but that would have a bigger impact on the expected cost of the method.

jglick · 2017-10-19T15:54:43Z

Yes it is possible to break cycles using ThreadLocal if it is considered important to keep this fallback. For purposes of this patch I wanted to just keep it simple and revert to the 2.84 behavior because I did not see any particular reason for it to have been added in the first place. @Jimilian please correct me if this part of the original patch was essential for some reason.

recampbell · 2017-10-19T15:59:07Z

Would be nice to have a test in core to make sure we don't regress this again.

Jimilian · 2017-10-19T16:27:55Z

@jglick I introduced it because if someone did not override getCauseOfBlockage it will just pass through all checks.

CauseOfBlockage causeOfBlockage = task.getCauseOfBlockage();
if (causeOfBlockage != null) { // here we need to check `task.isBlocked`
    return task.getCauseOfBlockage();

if (!canRun(task.getResourceList())) {
    ...
}

Now I think it was better to add a check for task.isBlocked in getCauseOfBlockageForItem instead of changing default implementation in first place...

jglick · 2017-10-19T16:36:06Z

Having both methods seems like a bad design in the first place. Maybe we can simply deprecate isBuildBlocked (core no longer calls it anyway) and have it delegate to getCauseOfBlockage() != null. I think that would be more compatible. Let me try it.

…ge, which you needed to implement anyway.

Materially changed since review.

jglick · 2017-10-19T17:09:20Z

core/src/main/java/hudson/model/AbstractProject.java

-     * A project must be blocked if its own previous build is in progress,
-     * or if the blockBuildWhenUpstreamBuilding option is true and an upstream
-     * project is building, but derived classes can also check other conditions.
-     */


Now no-op overrides.

jglick · 2017-10-19T17:10:02Z

core/src/main/java/hudson/model/Queue.java

-         *
-         * <p>
-         * Short-hand for {@code getCauseOfBlockageForItem()!=null}.
+         * @deprecated Use {@link #getCauseOfBlockage} != null


As of 2.85 was already not being called by core, so why keep it?

Should it be also restricted?

jglick · 2017-10-19T17:10:28Z

core/src/main/java/hudson/model/Queue.java

         */
        @Deprecated
-        String getWhyBlocked();
+        default String getWhyBlocked() {


Some other methods in this interface could benefit from default implementations but I will leave that to another day.

jglick · 2017-10-19T17:10:38Z

core/src/main/java/hudson/model/Queue.java

         */
        @CheckForNull
        default CauseOfBlockage getCauseOfBlockage() {
-            if (isBuildBlocked()) {


jglick · 2017-10-19T17:11:33Z

core/src/test/java/hudson/model/queue/AbstractQueueTaskTest.java

+    @Test
+    public void causeOfBlockageOverrides() {
+        Queue.Task t = new LegacyTask();
+        assertFalse(t.isBuildBlocked());


With Queue.java reverted to master version, reproduces the StackOverflowError.

jglick · 2017-10-19T17:12:07Z

core/src/test/java/hudson/model/queue/AbstractQueueTaskTest.java

+        assertNull(t.getWhyBlocked());
+        assertNull(t.getCauseOfBlockage());
+    }
+    static class LegacyTask extends AbstractQueueTask {


Same essential structure as current org.jenkinsci.plugins.workflow.job.AfterRestartTask.

jglick · 2017-10-19T17:13:52Z

test/src/test/java/hudson/model/QueueTest.java

        @Override public int hashCode() {
            return cnt.hashCode();
        }
-        @Override public boolean isBuildBlocked() {return isBlocked;}


This was only a test task. A real one would already have needed to implement getCauseOfBlockage to provide a message to the user. (It was abstract prior to #2879 in 2.62 so it is unlikely that new code was written against 2.62–2.85 that assumed that only implementing isBuildBlocked was OK.)

jglick · 2017-10-19T17:15:27Z

test/src/test/java/hudson/model/QueueTest.java

-        queue.maintain();
-
-        assertEquals(1, r.jenkins.getQueue().getBlockedItems().size());
-        CauseOfBlockage actual = r.jenkins.getQueue().getBlockedItems().get(0).getCauseOfBlockage();


Testing something which is no longer wanted.

oleg-nenashev

🐝 Maybe needs some polishing later, but it seems to be a good fix for now

oleg-nenashev · 2017-10-20T16:56:53Z

core/src/main/java/hudson/model/Queue.java

-         *
-         * <p>
-         * Short-hand for {@code getCauseOfBlockageForItem()!=null}.
+         * @deprecated Use {@link #getCauseOfBlockage} != null


Should it be also restricted?

oleg-nenashev · 2017-10-20T16:59:10Z

I assume @Jimilian also approved it, am I right?

Jimilian · 2017-10-20T17:46:07Z

@oleg-nenashev, yes, fix itself looks fine for me. But I'm not 100% sure that it handles mentioned problem whenisBuildBlocked was overriden, but getCauseOfBlockage() - was not. Because isBuildBlocked is not called anymore, so, if old plugin that relays only on isBuildBlocked will be executed on new core -> it will just pass through the check as default implementation of getCauseOfBlockage() returns null.

p.s. Maybe I still do not get classpath/api magic in Jenkins.

jglick · 2017-10-20T18:20:10Z

Should it be also restricted?

No. Why? @Restricted is for things which must be public (or protected) for technical reasons but should not be considered part of the API. @Deprecated is for things which used to be part of the API but should no longer be used. They do not overlap.

jglick · 2017-10-20T18:22:23Z

I'm not 100% sure that it handles mentioned problem when isBuildBlocked was overridden, but getCauseOfBlockage() was not.

No, it does not. Already discussed above in #3099 (comment). I do not consider it a problem.

stephenc · 2017-10-20T18:24:49Z

@Restricted will break the build on upgrade and accelerate the avoidance of the method. @Deprecated can (and consequently will) be easily ignored.

I think adding a new type of restriction, @Restricted(Legacy) would be the way to go

jglick · 2017-10-20T18:35:33Z

@Restricted will break the build on upgrade and accelerate the avoidance of the method.

At the cost of interrupting unrelated developer workflows, such as

mvn -Djenkins.version=2.99 clean test

Anyway I see no reason why avoidance of this method needs to be accelerated beyond any other deprecated API.

@Deprecated can (and consequently will) be easily ignored.

It can be, yes, but that is a separate issue, solvable using standard techniques such as compiler flags, or the Warnings plugin for Jenkins for that matter.

jglick · 2017-10-20T20:38:54Z

@reviewbybees done

jglick · 2017-10-20T22:02:37Z

Merging this to make sure it gets into 2.86 since I think the StackOverflowError is too severe to let sit. Follow-ups are possible if necessary.

oleg-nenashev · 2017-10-21T07:28:49Z

Thanks @jglick !

[JENKINS-47517] Revert the change to the default behavior of Queue.Ta…

0e5274b

…sk.getCauseOfBlockage.

jglick requested review from daniel-beck and oleg-nenashev October 19, 2017 02:09

svanoort previously approved these changes Oct 19, 2017

View reviewed changes

stephenc previously approved these changes Oct 19, 2017

View reviewed changes

jglick added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Oct 19, 2017

Deprecate isBuildBlocked & getWhyBlocked in favor of getCauseOfBlocka…

19714f2

…ge, which you needed to implement anyway.

jglick added needs-more-reviews Complex change, which would benefit from more eyes and removed ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback labels Oct 19, 2017

jglick commented Oct 19, 2017

View reviewed changes

jglick mentioned this pull request Oct 19, 2017

Removing some no-op overrides jenkinsci/workflow-job-plugin#79

Merged

oleg-nenashev approved these changes Oct 20, 2017

View reviewed changes

oleg-nenashev self-assigned this Oct 20, 2017

svanoort approved these changes Oct 20, 2017

View reviewed changes

jglick added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Oct 20, 2017

jglick merged commit 5e6b105 into jenkinsci:master Oct 20, 2017

jglick deleted the getCauseOfBlockage-JENKINS-47517 branch October 20, 2017 22:02

basil mentioned this pull request Jan 23, 2019

Remove some no-op overrides jenkinsci/workflow-job-plugin#123

Merged

Uh oh!

[JENKINS-47517] Revert the change to the default behavior of Queue.Task.getCauseOfBlockage #3099

[JENKINS-47517] Revert the change to the default behavior of Queue.Task.getCauseOfBlockage #3099

Uh oh!

Conversation

jglick commented Oct 19, 2017

Proposed changelog entries

Uh oh!

ghost commented Oct 19, 2017

Uh oh!

svanoort left a comment

Choose a reason for hiding this comment

Uh oh!

Jimilian commented Oct 19, 2017

Uh oh!

jglick commented Oct 19, 2017

Uh oh!

jglick commented Oct 19, 2017

Uh oh!

daniel-beck commented Oct 19, 2017

Uh oh!

stephenc commented Oct 19, 2017

Uh oh!

jglick commented Oct 19, 2017

Uh oh!

recampbell commented Oct 19, 2017

Uh oh!

Jimilian commented Oct 19, 2017 • edited by jglick Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jglick commented Oct 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev commented Oct 20, 2017

Uh oh!

Jimilian commented Oct 20, 2017

Uh oh!

jglick commented Oct 20, 2017

Uh oh!

jglick commented Oct 20, 2017

Uh oh!

stephenc commented Oct 20, 2017

Uh oh!

jglick commented Oct 20, 2017

Uh oh!

jglick commented Oct 20, 2017

Uh oh!

jglick commented Oct 20, 2017

Uh oh!

oleg-nenashev commented Oct 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Jimilian commented Oct 19, 2017 •

edited by jglick

Loading