[
https://issues.apache.org/jira/browse/GROOVY-8097?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15899553#comment-15899553
]
Ion Alberdi edited comment on GROOVY-8097 at 3/7/17 2:56 PM:
-------------------------------------------------------------
[~jexler] Regarding GROOVY_7404, indeed, the proposed fix consists at making
Ivy use a different resolutionCacheDir. As far as I understood, this requires
to have a different GrapeIvy instance per concurrent Grab (each with a
different ivySetting). It may not necessarily require to load a different
GrapeIvy.class, but to use different GrapeIvy objects instead.
The first idea in the PR proposal consisted at
1. enabling @Grab(group='foo', module='bar', version='1.0',
resolutionCacheDir='/someCacheDir'),
2. GrabAnnotationTransformation.java will call Grape.grab with the additional
argument resolutionCacheDir,
3. Grape will switch from a singleton to a map of <string: resolutionCacheDir,
GrapeIvy.class: grapeIvy,> each grapeIvy pointing to a different
resolutionCacheDir.
I don't see for now how to transfer this information from Grab to GrapeIvy
without tainting Grab with Ivy. And I agree this is something that should be
avoided.
Do we have other arguments available in Grab that would permit implementing
such a functionality ?
As a side note, we indeed depend on the @Grab notation as the import done in
the groovy will raise ClassNotFound exception else. We thought about
implementing a method that grabs with GrapeIvy, and calls a closure containing
all the imports after, but if possible, the @Grab solution seems preferable, as
other developers wouldn't need to re-implement a similar closure based method.
Regarding
"Ah, maybe a system property that "tells" each loaded GrapeIvy.class to
choose/create its own random resolution cache directory as a subdirectory of
the directory given in the system property would a nice way to handle this
issue?" As far I understood a single GrapeIvy.class instance is used for now,
and for this reason a single resolutionCacheDirectory can be used in the same
JVM. For this reason, I don't think the random resolution cache directory could
work, but I may be wrong (this is the first time I ever read Groovy's tree).
As a side-note, the randomness may greatly reduce the occurence of the
concurrency issue, whereas the per concurrent job resolutionCacheDir will solve
it.
[~blackdrag] The experiment we did (see "Protect the calls to grab with a lock
similar to ivy's "artifact-lock-nio" strategy. Works but slow." above) confirms
that a synchronized call to grab solves the issue. However this resulted in
significantly slower resolutions (we did not measure it though). I'm afraid
that implementing this synchronization, a global "artifact-lock-nio" put aside,
may not be so easy to implement (Ivy may have already done it otherwise).
was (Author: yetanotherion):
[~jexler] Regarding GROOVY_7404, indeed, the proposed fix consists at making
Ivy use a different resolutionCacheDir. As far as I understood, this requires
to have a different GrapeIvy instance per concurrent Grab (each with a
different ivySetting). It may not necessarily require to load a different
GrapeIvy.class, but to use different GrapeIvy objects instead.
The first idea in the PR proposal consisted at
1. enabling @Grab(group='foo', module='bar', version='1.0',
resolutionCacheDir='/someCacheDir'),
2. GrabAnnotationTransformation.java will call Grape.grab with the additional
argument resolutionCacheDir,
3. Grape will switch from a singleton to a map of <string: resolutionCacheDir,
GrapeIvy.class: grapeIvy,> each grapeIvy pointing to a different
resolutionCacheDir.
I don't see for now how to transfer this information from Grab to GrapeIvy
without tainting Grab with Ivy. And I agree this is something that should be
avoided.
Do we have other arguments available in Grab that would permit implementing
such a functionality ?
As a side note, we indeed depend on the @Grab notation as the import done in
the groovy will raise ClassNotFound exception else. We thought about
implementing a method that grabs with GrapeIvy, and calls a closure containing
all the imports after, but if possible, the @Grab solution seems preferable, as
other developers wouldn't need to re-implement a similar closure based method.
Regarding
"Ah, maybe a system property that "tells" each loaded GrapeIvy.class to
choose/create its own random resolution cache directory as a subdirectory of
the directory given in the system property would a nice way to handle this
issue?" As far I understood a single GrapeIvy.class instance is used for now,
and for this reason a single resolutionCacheDirectory can be used in the same
JVM. For this reason, I don't think the random resolution cache directory could
work, but I may be wrong (this is the first time I ever read Groovy's tree).
As a side-note, the randomness may greatly reduce the occurence of the
concurrency issue, whereas the per concurrent job resolutionCacheDir will solve
it.
[~blackdrag] The experiment we did (see "Protect the calls to grab with a lock
similar to ivy's "artifact-lock-nio" strategy. Works but slow." above) confirms
that a synchronized call to grab solves the issue. However this resulted in
significantly slower resolutions (we did not measure it though). I'm afraid
that this synchronization, a global "artifact-lock-nio" put aside, may not be
so easy to implement (Ivy would have already done it otherwise).
> Add an argument to set the resolution cache path in @Grab
> ---------------------------------------------------------
>
> Key: GROOVY-8097
> URL: https://issues.apache.org/jira/browse/GROOVY-8097
> Project: Groovy
> Issue Type: New Feature
> Components: Grape
> Affects Versions: 2.4.8
> Reporter: Ion Alberdi
> Priority: Minor
>
> Ivy does not support concurrent access to its resolution cache
> https://issues.apache.org/jira/browse/IVY-654
> Grape relies on Ivy. For this reason, Grape cannot support concurrent access
> to its resolution cache neither.
> When using the @Grab annotation in jenkins groovyCommand or
> systemGroovyCommand, the related code is vulnerable to race conditions. When
> the race condition appears in a systemGroovyCommand, we have no choice but to
> reboot jenkins as all consecutive calls to @Grab fail.
> Among the two solutions we tried:
> - Protect the calls to grab with a lock similar to ivy's "artifact-lock-nio"
> strategy. Works but slow.
> - Set Ivy's lock on the repository cache and setup Grab to use a different
> cache resolution cache for each concurrent jobs. The following code permits
> to fix a test we did to reproduce the race condition.
> {code}
> static IvySettings createIvySettings(String resolutionPath, boolean
> dumpSettings) {
> // Copy/Paste/Purged from GrapeIvy.groovy
> IvySettings settings = new IvySettings()
> settings.load(new File(GROOVY_HOME, "grapeConfig.xml"))
> // set up the cache dirs
> settings.defaultCache = new File(GRAPES_HOME)
> settings.setVariable("ivy.default.configuration.m2compatible", "true")
> settings.setDefaultResolutionCacheBasedir(resolutionPath)
> return settings
> }
> static GrapeIvy ivyWithCustomResolutionPath(String resolutionPath) {
> Class<?> grapeIvyClass = Class.forName("groovy.grape.GrapeIvy");
> Object instance = grapeIvyClass.newInstance()
> Field field = grapeIvyClass.getDeclaredField("ivyInstance");
> field.setAccessible(true);
> field.set(instance,
> Ivy.newInstance(createIvySettings(resolutionPath)));
> return ((GrapeIvy)instance)
> }
> {code}
> We'd like to propose to add an additional argument to Grab to setup Ivy's
> resolution cache directory.
> Note that this solution seems to have been adopted by these users too
> https://rbcommons.com/s/twitter/r/3436/
> Would you agree on such a feature ? We'd be glad to propose a PR.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
