groovy-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ion Alberdi (JIRA)" <>
Subject [jira] [Commented] (GROOVY-8097) Add an argument to set the resolution cache path in @Grab
Date Tue, 07 Mar 2017 14:53:41 GMT


Ion Alberdi commented on GROOVY-8097:

[~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 does not necessarily require to
load a different GrapeIvy.class, but to use different GrapeIvy objects.

The first idea in the PR proposal consisted at 
1. enabling @Grab(group='foo', module='bar', version='1.0', resolutionCacheDir='/someCacheDir'),

2. 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 require to re-implement a similar
closure based method.

"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:
>             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 
> 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
> Would you agree on such a feature ? We'd be glad to propose a PR.

This message was sent by Atlassian JIRA

View raw message