[JENKINS-60997] Work around BEANUTILS-509 #110
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commons BeanUtils apparently has a per thread context classloader (!) data structure that is not thread-safe, which is a correctness issue because the class loader can be shared between multiple threads. Upstream attempts to resolve this issue seem to have stalled over the years.
I audited
WrapDynaBean
and the only code that seems to read or write to this data structure in practice isorg.apache.commons.beanutils.WrapDynaClass#createDynaClass
(org.apache.commons.beanutils.WrapDynaClass#clear
andorg.apache.commons.beanutils.WrapDynaClass#dynaClasses
can theoretically read or write to it, but they aren't called anywhere in practice). So serializing access toorg.apache.commons.beanutils.WrapDynaClass#createDynaClass
should ensure that no two callers interfere with each other.This PR achieves that objective with a simple
private static final
(i.e., per-classloader) lock. This does not improve performance, but remember, we are facing a correctness bug, and this does increase correctness. And the performance penalty is negligible—per this page:Even with a global lock, I can't imagine enough people rendering Jelly views concurrently for this to be an issue. There would have to be thousands of Jelly views being rendered concurrently, at which point it seems like this global lock would be the least of your worries. Also, the operation under the critical section is just CPU and memory-bound (not I/O), so it should run in a breeze on modern hardware.
This could likely be optimized further, for example by introducing our own
ClassValue
-based cache ofDynaBean
instances, but such optimization seems unnecessary in the absence of any evidence that this is a performance bottleneck. Correctness first, then performance. (This PR is about the former.)Testing done
I tested this by deploying this change to a local controller and running a few Pipeline jobs with no issues.