[JENKINS-46386] - Detach ServletContext logic to a pluggable Provider #3065

oleg-nenashev · 2017-10-09T08:19:43Z

It should prevent classloading issues on agents for sure. The implementation is a bit hackish, ideally providers should be somehow integrated into the Jenkins object singleton (though then we need to prevent variable usage before Jenkins' full initialization)

No autotests so far, because it requires specification of a custom web.xml for the WAR file. Will tests the case manually

See JENKINS-46386.

Proposed changelog entries

Bug: JENKINS-46386 - Prevent failure of custom agent types with javax.servlet.ServletContextListenerclassloading issues. Now the class is not being propagated to agents
...

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

@reviewbybees @KostyaSha

It should prevent classloading issues on agents for sure.

ghost · 2017-10-09T08:21:43Z

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.

…l list

jtnord · 2017-10-09T17:02:58Z

core/src/main/java/jenkins/util/SystemPropertiesProvider.java

+
+    //TODO: This should use the annotation indexer at least.
+    // Ideally it should be a part of the Jenkins singleton in order to play nicely with {@code JenkinsRule}.
+    private static final CopyOnWriteMap<String, SystemPropertiesProvider> providers = new CopyOnWriteMap.Hash<>();


the map symatics are not really unused - you could remove using the actual provider (object) to remove and use a standard CopyOnWrite collection.

Ontop of this you get different (random) iteration order (the CopyOnWriteMap makes no guarantees about ordering). so could get hard to reproduce bugs when you sometimes get a system property and other times get a ServletContext parameter...

Yeah, probably Map is not required. I just added it to support Removal, but there are more simple and efficient ways for sure

Well, this is all restricted, and there is no removal, so KISS.

jtnord

left a comment about what looks like a bug inline.

ontop of this I wonder if anyone really uses this functionality. Would a flat properties file in JENKINS_HOME (CWD of the agent) not be much better and would work on agents and masters alike?

(OT) The fact the jenkins changes behaviour based on unversioned environment variables also troubles me (I still see people starting services with /etc/initi.d/servericec start... which will not start with a clean environment but whatever the current user has polluted their env with to begin with

oleg-nenashev · 2017-10-09T17:17:26Z

ontop of this I wonder if anyone really uses this functionality. Would a flat properties file in JENKINS_HOME (CWD of the agent) not be much better and would work on agents and masters alike?

Yes, I confirm I use these settings to customize the Jenkins behavior from my custom Jenkins WAR packaging. More in the private chat.

The fact the jenkins changes behaviour based on unversioned environment variables also troubles me

I suggest filing a ticket

oleg-nenashev · 2017-10-09T17:19:12Z

I need to fix the test codebase. Yes, I will try compiling it on my machine next time...

…INS-46386-systemPropertiesProvider

jglick · 2017-10-16T14:09:41Z

The broader issue is that #3067 lacked a test because

agents will only be unable to load javax.servlet.* under certain servlet containers or environments, IIUC
Jenkins fails to block RemoteClassLoader from even trying to load such classes under normal conditions
thus, routine test suites which do simple stuff on agents pass despite the bug

This is long filed in JIRA and I would rather prioritize a fix of that so we are not making speculative and untestable fixes.

oleg-nenashev · 2017-10-16T14:22:57Z

Yes, even on the wild the issue happens rarely. @KostyaSha got a reproducible scenario in YAD, but I was using the plugin easily for many containers without an issue. ATH does not catches it as well. @jglick are you fine with the current fix PoC in this PR?

For sure, we need to make the classloading logic more efficient + maybe add some static analysis around the OnMaster interface. It's not the only place in the code we may get into such misbehavior, unfortunately.

jglick

A bit overengineered, and still does nothing to block us from making the broader category of mistakes in the future (nor is there any test proving that the fix works), but seems to be a step in the right direction.

jglick · 2017-10-27T19:13:42Z

core/src/main/java/jenkins/util/ServletContextSystemPropertiesProvider.java

+     */
+    @Override
+    @SuppressFBWarnings(value = "ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD",
+            justification = "Currently Jenkins instance may have one ond only one context")


jglick · 2017-10-27T19:14:22Z

core/src/main/java/jenkins/util/ServletContextSystemPropertiesProvider.java

+     * The ServletContext to get the "init" parameters from.
+     */
+    @CheckForNull
+    private static ServletContext theContext;


Why is this static?

jglick · 2017-10-27T19:15:50Z

core/src/main/java/jenkins/util/SystemPropertiesProvider.java

+ * @see ServletContextSystemPropertiesProvider
+ */
+@Restricted(NoExternalUse.class)
+public abstract class SystemPropertiesProvider {


This and ServletContextSystemPropertiesProvider could have just have been nested classes of SystemProperties, I think; would be more readable.

jglick · 2017-10-27T19:17:14Z

core/src/main/java/jenkins/util/SystemPropertiesProvider.java

+
+    //TODO: This should use the annotation indexer at least.
+    // Ideally it should be a part of the Jenkins singleton in order to play nicely with {@code JenkinsRule}.
+    private static final CopyOnWriteMap<String, SystemPropertiesProvider> providers = new CopyOnWriteMap.Hash<>();


Well, this is all restricted, and there is no removal, so KISS.

jglick · 2017-10-27T19:18:02Z

core/src/main/java/jenkins/util/SystemPropertiesProvider.java

+     * Gets all registered providers.
+     * @return Collection of registered providers.
+     */
+    public static Collection<SystemPropertiesProvider> all() {


Restricted anyway

jglick · 2017-10-27T19:18:05Z

core/src/main/java/jenkins/util/SystemPropertiesProvider.java

+     * @return Removed provider (if any)
+     */
+    @CheckForNull
+    public static SystemPropertiesProvider removeProvider(@Nonnull Class<?> providerClass) {


jglick · 2017-10-27T19:19:51Z

test/src/test/java/jenkins/util/ServletContextSystemPropertiesProviderTest.java

+    @Before
+    public void setUp() {
+        // Does not matter what we call, the fields are static
+        new ServletContextSystemPropertiesProvider().contextInitialized(new ServletContextEvent(j.jenkins.servletContext));


Why would this not be called automatically by Jetty during JenkinsRule setup?

jglick · 2017-10-27T19:22:42Z

Except:

Write to static field jenkins.util.ServletContextSystemPropertiesProvider.theContext from instance method jenkins.util.ServletContextSystemPropertiesProvider.contextDestroyed(ServletContextEvent) [jenkins.util.ServletContextSystemPropertiesProvider] At ServletContextSystemPropertiesProvider.java:[line 66] ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD

as I noted.

oleg-nenashev · 2017-10-28T11:10:04Z

Needs fix, yes. Due to whatever reason, FindBugs behaves differently in CI

oleg-nenashev · 2018-03-23T12:05:20Z

After #3362 this fix is not actual, but it still makes sense to revive it once we need better extensibility

stephenc

Is this still relevant?

oleg-nenashev · 2019-01-02T10:23:11Z

unlikely

batmat · 2019-01-02T15:20:01Z

@oleg-nenashev also, it's conflicted. What do you think about closing it?

oleg-nenashev · 2019-01-02T15:21:43Z

Aside grumbling about priority switches and lots of almost-done work being abandoned, I am fine with that. Closing it

[JENKINS-46386] - Detach ServletContext logic to a pluggable Provider

66b6bde

It should prevent classloading issues on agents for sure.

oleg-nenashev added 2 commits October 9, 2017 10:22

[JENKINS-46386] - Also allow removing providers and getting their ful…

ce703d6

…l list

[JENKINS-46386] - SystemProperties now can be safely used on agents

f07b4e7

oleg-nenashev mentioned this pull request Oct 9, 2017

[JENKINS-46386] - Don't use unloadable things in remoting mkdirs calls. #3067

Merged

oleg-nenashev requested a review from stephenc October 9, 2017 09:38

[JENKINS-46386] - Thanks FindBugs

6ac996b

daniel-beck self-requested a review October 9, 2017 11:56

oleg-nenashev requested a review from jtnord October 9, 2017 15:40

jtnord reviewed Oct 9, 2017

View reviewed changes

oleg-nenashev added the work-in-progress The PR is under active development, not ready to the final review label Oct 9, 2017

oleg-nenashev added 2 commits October 13, 2017 12:01

Merge commit '62c4408c91b37767d700e84b87fed720426349ad' into bug/JENK…

3059063

…INS-46386-systemPropertiesProvider

[JENKINS-46386] - Cleanup Tests

ad9c95e

oleg-nenashev added needs-more-reviews Complex change, which would benefit from more eyes and removed work-in-progress The PR is under active development, not ready to the final review labels Oct 13, 2017

jglick self-requested a review October 26, 2017 15:42

jglick approved these changes Oct 27, 2017

View reviewed changes

oleg-nenashev self-assigned this Oct 28, 2017

jglick mentioned this pull request Mar 21, 2018

[JENKINS-46386] Make SystemProperties safe to call from agent JVMs #3362

Merged

oleg-nenashev added the unresolved-merge-conflict There is a merge conflict with the target branch. label Mar 23, 2018

stephenc reviewed Nov 16, 2018

View reviewed changes

batmat added the proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it label Jan 2, 2019

oleg-nenashev closed this Jan 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-46386] - Detach ServletContext logic to a pluggable Provider #3065

[JENKINS-46386] - Detach ServletContext logic to a pluggable Provider #3065

oleg-nenashev commented Oct 9, 2017

ghost commented Oct 9, 2017

jtnord Oct 9, 2017 •

edited

Loading

oleg-nenashev Oct 9, 2017

jglick Oct 27, 2017

jtnord left a comment

oleg-nenashev commented Oct 9, 2017

oleg-nenashev commented Oct 9, 2017

jglick commented Oct 16, 2017

oleg-nenashev commented Oct 16, 2017

jglick left a comment

jglick Oct 27, 2017

jglick Oct 27, 2017

jglick Oct 27, 2017

jglick Oct 27, 2017

jglick Oct 27, 2017

oleg-nenashev Oct 28, 2017

jglick Oct 27, 2017

jglick Oct 27, 2017

jglick commented Oct 27, 2017

oleg-nenashev commented Oct 28, 2017

oleg-nenashev commented Mar 23, 2018

stephenc left a comment

oleg-nenashev commented Jan 2, 2019

batmat commented Jan 2, 2019

oleg-nenashev commented Jan 2, 2019

[JENKINS-46386] - Detach ServletContext logic to a pluggable Provider #3065

[JENKINS-46386] - Detach ServletContext logic to a pluggable Provider #3065

Conversation

oleg-nenashev commented Oct 9, 2017

Proposed changelog entries

Submitter checklist

Desired reviewers

ghost commented Oct 9, 2017

jtnord Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Oct 9, 2017

oleg-nenashev commented Oct 9, 2017

jglick commented Oct 16, 2017

oleg-nenashev commented Oct 16, 2017

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Oct 27, 2017

oleg-nenashev commented Oct 28, 2017

oleg-nenashev commented Mar 23, 2018

stephenc left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Jan 2, 2019

batmat commented Jan 2, 2019

oleg-nenashev commented Jan 2, 2019

jtnord Oct 9, 2017 •

edited

Loading