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

[JENKINS-60449] Fix and edge case of OptionalExtension blowing up Jenkins #4393

Merged
merged 4 commits into from
Jan 3, 2020

Conversation

Vlatombe
Copy link
Member

@Vlatombe Vlatombe commented Dec 11, 2019

See JENKINS-60449.

Includes binaries from ikedam/dependencytest#2

If you have an extension like this

@OptionalExtension(requirePlugins = {"dependee"})
public class OptionalDependerExtension {
    private static void foo(Dependee d) {

    }
}

and dependee is not loaded initially, but you load it dynamically later, this blows up, because when Guice does a method scan and can't resolve Dependee because it is not yet in classloader at the time of scanning.

This PR can be reviewed commit by commit.

  • Introduce a failing test
  • Introduce functional changes fixing the test

Proposed changelog entries

  • Entry 1: Fix an edge case of loading optional dependencies that cause Jenkins to blow up
  • ...

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

@Vlatombe Vlatombe requested a review from jglick December 11, 2019 17:57
* Variant plugin is a dependency of optional-depender

Optional-depender contains the following class

@OptionalExtension(requirePlugins = {"dependee"})
public class OptionalDependerExtension {
      private static void foo(Dependee d) {

      }
}
@Vlatombe
Copy link
Member Author

@jglick Updated the PR with a simpler diff (no more refactoring, and a new failing test instead of updating an existing one)

@Vlatombe Vlatombe marked this pull request as ready for review December 12, 2019 11:25
Inspect all declared constructors/methods/fields recursively to exclude
failing classes from Guice rather than have Guice explode.

This also fix the failing test, but the previous commit is still better
because it avoids an extra Jenkins.get().refreshExtensions() call.
@res0nance res0nance added the bug For changelog: Minor bug. Will be listed after features label Dec 19, 2019
@oleg-nenashev oleg-nenashev requested a review from a team December 20, 2019 10:21
Copy link
Member

@halkeye halkeye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so over my head, so as a core pr reviewer I'm going to abstain from this one but its cool to try to figure out what's going on

+1 for tests

@fcojfernandez fcojfernandez requested a review from a team December 26, 2019 07:50
Copy link
Contributor

@fcojfernandez fcojfernandez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, code looks good to me

@fcojfernandez fcojfernandez requested a review from a team December 26, 2019 07:57
@fcojfernandez fcojfernandez added the needs-more-reviews Complex change, which would benefit from more eyes label Dec 26, 2019
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine though it would be great to run PCT against the patched version

@jglick
Copy link
Member

jglick commented Jan 3, 2020

Guice does a method scan and can't resolve Dependee

By the way: google/guice#1283

@jglick 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 Jan 3, 2020
@oleg-nenashev oleg-nenashev merged commit dd59e8c into jenkinsci:master Jan 3, 2020
@oleg-nenashev oleg-nenashev changed the title [JENKINS-60449] Attempt to fix an odd case of OptionalExtension blowing up Jenkins [JENKINS-60449] Fix and edge case of OptionalExtension blowing up Jenkins Jan 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For changelog: Minor bug. Will be listed after features ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants