-
Notifications
You must be signed in to change notification settings - Fork 165
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
Overhauled ProxyWhitelist
#549
Conversation
|
||
@Override public final boolean permitsMethod(@NonNull Method method, @NonNull Object receiver, @NonNull Object[] args) { | ||
String key = canonicalMethodSig(method); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this will no longer cache results when there is a distinct Method
with the same signature, which could happen when there is a new copy of the Class
from a distinct ClassLoader
. However https://github.com/jenkinsci/workflow-cps-plugin/blob/3a59e40fb41d2774421ce3d7c00480e5917edb76/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovyClassLoaderWhitelist.java#L45 consults its delegate
https://github.com/jenkinsci/workflow-cps-plugin/blob/3a59e40fb41d2774421ce3d7c00480e5917edb76/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsGroovyShell.java#L181 last so I do not think that would normally happen. Once the system warms up, the normal path through ought to be fast. Would be useful to have some JMH benchmarks if they can run via RealJenkinsRule
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks a lot simpler
…ere are no signatures; avoiding `Stream.anyMatch`
… prior to some other simplification, but not now
I would like to do some sort of performance sanity check of this. Also, it is probably not the best idea to release a rewrite like this right before taking time off! |
Installing Log
|
Set up a controller running 3872d58 with 10 (mock agent) executors and a pipeline with a string parameter library 'github.com/jglick/sample-pipeline-library'
def changes = currentBuildExt().hasChangeIn('xxx')
echo """
Hello at ${new Date()}!
Triggered as $x
Do we have any changes? $changes
"""
node {
parallel main: {
stage('Build') {
withMockLoad(averageDuration: 3) {
sh MOCK_LOAD_COMMAND
}
}
stage('Results') {
junit 'mock-junit.xml'
archiveArtifacts 'mock-artifact-*.txt'
}
}, stuff: {
sh 'sleep 3'
echo(/hello from ${Jenkins.instance}/) // approved
}
}
def r = new Random()
['a', 'b'].each {one ->
def p = r.nextInt(20)
echo "$one triggering at $p"
build job: JOB_NAME, wait: false, parameters: [string(name: 'x', value: String.valueOf(p))]
} I saw no evidence of classes from this plugin in thread dumps, other than momentarily while processing the |
private final Map<ProxyWhitelist,Void> wrappers = new WeakHashMap<>(); | ||
|
||
// TODO Consider StampedLock when we switch to Java8 for better performance - https://dzone.com/articles/a-look-at-stampedlock | ||
private ReentrantReadWriteLock lock = new ReentrantReadWriteLock(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I think the old code would have blocked permits*
calls from other threads while ApprovedWhitelist
was being reconfigured, while the new code with the lock removed reconfigures things on the first thread that sees ApprovedWhitelist.initialized
change from false
to true
, and other threads will not be blocked and will either use the old or new list depending on timing. Seems ok to me, but I just wanted to note the behavior change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I think the lock was there merely to prevent momentary inconsistencies, not specifically to block permit*
calls.
Had a lot of problems running ATH due to mock UC issues, e.g.
Had to manually delete local repository files. Finally managed to get Guice error
and then failing in the test proper Assertion failure
which is fixed using |
Fixes jenkinsci/acceptance-test-harness#1444. (I guess; I do not care to run ATH locally, but anyway the problenatic code is completely rewritten so it ought to be possible to revert jenkinsci/acceptance-test-harness#1445.) Motivated by #545 (comment): there was some code that attempted to work around Guice cycles, but I guess it behaved nondeterministically depending on extension load order. Simplified the whole thing, which required rewriting
ProxyWhitelist
to be much simpler and not try to cache anything, and rewritingEnumeratingWhitelist
to use a simpler cache system (replacing #180 and various other accreted patches).