[ 
https://issues.apache.org/jira/browse/GROOVY-8097?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15899281#comment-15899281
 ] 

Jex Jexler commented on GROOVY-8097:
------------------------------------

Thanks for the additional context.

Regarding GROOVY-7407 and "my guess is that if threads use the same locked 
repositoryCacheDir and different resolutionCacheDir-s, the bug will not 
occur.": I would say that this is only really an option if the different 
threads each use a different class loader to load GrapeIvy.class resp. in 
practice to load "Groovy" (unless - theoretically - you would replace 
IvySettings on-the-fly in each thread and provide additonal locking around 
that). I guess I might still try this at one point just to find out to which 
degree the two issues are complementary.

Regarding GROOVY-8097: I see the point (and please note that I am not in any 
way officially involved with deciding about new Groovy features), but I am not 
sure if Grape would the best place to "fix" it.

For example, if you added a feature 
@GrabConfig(ivyResolutionCacheDir='/path/to/cache') to Grape, each call would 
make a global change in GrapeIvy.class, so if you had several scripts and 
utility classes in Groovy that use Grape, you would have to make sure they 
don't overwrite the settings. (Or you could only make the setting, the first 
time it is used in a script, but then you depend on that order, which in 
general can often be undefined when compiling.)

To me this is rather a setting you would make per loaded GrapeIvy.class, so 
probably also best to set it that way, as in the sample code in the description 
of this issue? (A system property would not be ideal for this, because you 
would not be able to dynamically select/create a directory easily and in 
general you would additionally have to lock grabs on a common class in the JDK 
or so with a mechanism similar to Grengine.Grape.activate(<some-jdk-class>).)

> 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)

Reply via email to