Skip to content

Conversation

@dwnusbaum
Copy link
Member

While looking for places to test jenkinsci/jenkins-test-harness#1029, I noticed that this class was allowing soft references while using MemoryAssert.assertGC, so I tracked them down and cleaned them out so the test can run with soft references disallowed.

Testing done

See updated tests.

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests that demonstrate the feature works or the issue is fixed

@dwnusbaum dwnusbaum requested a review from a team as a code owner September 30, 2025 21:44
Comment on lines +207 to +208
// Make sure ForkJoinPool is not initialized on a thread where its inheritedAccessControlContext would point to a CpsGroovyShell$CleanGroovyClassLoader
ForkJoinPool.commonPool().execute(() -> {});
Copy link
Member Author

@dwnusbaum dwnusbaum Sep 30, 2025

Choose a reason for hiding this comment

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

It's a bit interesting that I keep running into these cases where first-time initialization of various objects happening in the context of a Pipeline build causes problems. In practice it's probably ok, and at worst they would only cause 1-build leaks, but I wonder how many such leaks are actually possible.

Compare jenkinsci/pipeline-groovy-lib-plugin#199 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

There is a class loader sanity executor service wrapper in Jenkins core which should probably be used.

Copy link
Member Author

@dwnusbaum dwnusbaum Oct 1, 2025

Choose a reason for hiding this comment

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

Maybe, but this is the common pool, which seems to be unconfigurable, and the reference path was not via Thread.contextClassLoader as is typical (AppClassLoader in this case), but Thread.inheritedAccessControlContext, which holds a reference to CpsGroovyShell$CleanGroovyClassLoader via a ProtectionDomain.classloader, constructed I think via AccessController.getContext().

So whatever the exact problem is, it will go away once we pick up JEP 486 (Java 24+), which deletes Thread.inheritedAccessControlContext.

FWIW also I think the critical usage path of the common pool is via the default executor in Caffeine caches, specifically two caches in SimpleXStreamFlowNodeStorage and one in EnumeratingWhitelist. We could perhaps set their executors to Computer.threadPoolForRemoting or some manually-constructed thread pool to avoid the issue in the meantime.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, we ought to define a specific pool for use from Caffeine if possible. Ultimately using virtual threads (21+), though we might need to wait for monitor support in 24, and some diagnosability issues remain.

Copy link
Member Author

Choose a reason for hiding this comment

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

The notable thing in these cases is that we are not obviously using any async-related methods or types with the Caffeine caches. We only use weakKeys, strongValues, and LoadingCache, so the fact that an async task is being submitted to the common pool is not immediately obvious. Perhaps it has to do with the internal maintenance referred to in the Javadoc.

@jglick jglick enabled auto-merge September 30, 2025 22:20
@jglick jglick merged commit 0f6c0cf into jenkinsci:master Oct 1, 2025
16 of 17 checks passed
@dwnusbaum dwnusbaum deleted the clean-up-memoryassert-usage branch October 1, 2025 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants