[logback-user] JaninoEventEvaluator and class loader

[Glyn Normington]

Hi Ceki

Thanks for the detailed response! I raised LBCORE-244 and I'll definitely take a look at providing a patch as it would be good to validate the change under Virgo before it goes in. The gating factor will be how easy it is to build logback core, but I'm optimistic.

Regards,
Glyn

On 5 Jan 2012, at 22:47, ceki wrote:

> Hi Glyn,
> 
> We started moved away from ExpressionEvaluator and started using ScriptEvaluator because the latter allows for java blocks whereas the former allows only boolean expressions. The ability to parse java blocks reduces the ease-of-use gap between JaninoEvaluator and GEvaluator. See also:
> http://mailman.qos.ch/pipermail/logback-user/2011-January/002002.html
> 
> Class loading issues were not taken into consideration when making this change.
> 
> More in line.
> 
> On 05.01.2012 13:09, Glyn Normington wrote:
>> I am attempting to get Janino 2.6.1 working with Logback 0.9.28 (or
>> later, but that's the version we are using in Eclipse Virgo right now) -
>> see [0] for background. Unfortunately Janino ends up using the thread
>> context class loader as its "parent" class loader and fails with a
>> runtime exception. I have discussed this ([1]) on the Janino mailing
>> list and it seems that it is necessary to set the parent class loader in
>> Janino. I am somewhat at the mercy of Logback here.
> 
> OK.
> 
>> The surprising thing is that the code in Logback 0.9.24 looked pretty
>> usable in this respect. JaninoEventEvaluator.start had the following
>> sequence:
>> 
>> ClassLoader cl = context.getClass().getClassLoader();
>> ee = new ExpressionEvaluator(getDecoratedExpression(), EXPRESSION_TYPE,
>> getParameterNames(), getParameterTypes(), THROWN_EXCEPTIONS, cl);
>> 
>> thus setting the parent class loader to a value which I could ensure
>> would be capable of loading the necessary types.
>> 
>> In 0.9.28 this code has been replaced by:
>> 
>> scriptEvaluator = new ScriptEvaluator(getDecoratedExpression(),
>> EXPRESSION_TYPE,
>> getParameterNames(), getParameterTypes(), THROWN_EXCEPTIONS);
>> 
>> which causes the current TCCL to be used as the parent class loader and
>> ultimately results in Janino failing.
> 
> Well, as mentioned above, class loading issues were not taken into consideration when making the change. I was not aware that ScriptEvaluator used the TCCL by default.
> 
>> I can't control the TCCL that happens to be in use when start is called
>> as that is driven out of a logging call which can come from an arbitrary
>> thread.
> 
> Right.
> 
>> I found the relevant commit ([2]), but I can't tell from that why this
>> specific change was made.
> 
> Explanation give above.
> 
>> If it is absolutely necessary to use ScriptEvaluator rather than
>> ExpressionEvaluator, the following code sequence (based on a suggestion
>> from Arno Unkrig) would reproduce the parent class loader behaviour of
>> 0.9.24:
>> 
>> ClassLoader cl = context.getClass().getClassLoader();
>> scriptEvaluator = new ScriptEvaluator();
>> scriptEvaluator.setParentClassLoader(cl);
>> scriptEvaluator.setReturnType(EXPRESSION_TYPE);
>> scriptEvaluator.setParameterNames(getParameterNames());
>> scriptEvaluator.setParameterTypes(getParameterTypes());
>> scriptEvaluator.setThrownExceptions(THROWN_EXCEPTIONS);
>> scriptEvaluator.cook(getDecoratedExpression());
> 
> The above looks good to me. Please create a jira issue requesting for the above change. A reference to this message should provide the relevant context for the jira issue.
> 
>> The alternative of using ExpressionEvaluator is much neater and seems to
>> be close to the ScriptEvaluator variant since ExpressionEvaluator
>> extends ScriptEvaluator.
> 
> AFAIK, only ScriptEvaluator parses java blocks. ExpressionEvaluator does not.
> 
>> Any suggestions gratefully received.
> 
> I would not mind if in addition to the jira issue you could also apply and then test the approach suggested by Arno Unkrig, culminating in  git pull request. Yay!
> 
>> Regards,
>> Glyn
>> [0] https://bugs.eclipse.org/bugs/show_bug.cgi?id=333920
>> [1] mailing list archive link currently broken - if I can get a link to
>> it, I'll post this later
>> [2]
>> https://github.com/ceki/logback/commit/06a5b692f14560636bd92d7bd7cf1f85830f4e55#diff-4
>> 
> 
> 
> -- 
> Ceki
> http://twitter.com/#!/ceki
> _______________________________________________
> Logback-user mailing list
> Logback-user at qos.ch
> http://mailman.qos.ch/mailman/listinfo/logback-user