On 10/03/2016 21:16, [email protected] wrote:
> On Thursday, March 10, 2016 11:20 AM, [email protected] wrote:
>>
>>> 3. Why is the problem not limited to the first request for a jsp
>>> page?
>>
>> Because EL imports may be dynamic so the EL has to be evaluated on
>> execution.
>
> I'm not really sure I follow you now. Can you explain what you mean
> with dynamic imports in this regard? I can't see any mentioning of it
> in the specs
> (http://download.oracle.com/otn-pub/jcp/el-3_0-fr-eval-spec/EL3.0.FR.pdf).
There is nothing stopping a JSP author obtaining a reference to the
ImportHandler and conditionally adding classes to import. The
configuration of the ImportHandler could change on every call to the page.
> Can you give a concrete example, where an EL expression element
> would need to be evaluated as a potential class, again and again for
> each request?
See above.
> Because the way I see it, "foo" in ${foo.bar} in some jsp page only
> needs to be evaluated once. If "foo" corresponded to a class,
> matching one of the imports in the jsp, then this lookup will not
> change unless the jsp code changes. And the same is true if the
> lookup failes with a ClassNotFoundException. What am I missing?
See above.
>>> 4. Why isn't the ClassNotFoundException logged by the
>>> ImportHandler?
>>
>> Because it is expected and logging it provides no value to the
>> user.
>
> Well, I happen to disagree. :) In our case, if we could see all these
> stacktraces in the log, by enabling DEBUG/TRACE log level on the
> ImportHandler class for example, then we would quickly see just how
> big this problem is (ie how often this happens), on what jsp pages it
> happens, and what EL expressions cause it.
We'll have to agree to disagree on that one. If you are concerned about
a performance issue then you need to know where to look to enable debug
logging. A profiler will tell you where to look and at that point you
don't need the debug logging.
> On my local machine, I was able to "catch" the ClassNotFoundException
> using a debug breakpoint in my IDE, and using the breakpoint hit
> count I could see that this exception is thrown and handled (ie
> ignored) by the ImportHandler 2700 times for a single request to the
> start page.
>
>
>> The JavaEE specs are very big on backwards compatibility.
>> Therefore, the chances of changing the spec syntax to fix this are
>> zero.
>
> OK. Fair enough. I agree with you that backwards compatibility is
> important. But there are ways to fix this while still keeping the
> backwards compatibility. For example by making it possible to turn
> off this feature (globally, per webapp, or per jsp), where the
> default is "on". Wouldn't you agree that such a change would be 100%
> backwards compatible?
The code in question is in the JSP API JAR and there is no configuration
API available. The only option would be a system property which would
make it a global configuration option.
> And at the same time it would more or less
> solve this problem completely. Because people who experience the
> mentioned problems could turn of this setting globally, and then only
> enable it for those specific jsp pages where it is needed (and these
> pages would then be cleaned up, so that no EL-references exist
> without specific scope unless they are known to never be null. Tada,
> problem solved! =)
Um, no. See above. This would have to be a global option. It would work
for some users/pages but not for others.
> Actually... this wouldn't even need to be in the specifications... I
> can't see any harm in a EL implementation introducing such settings
> on its own, before the specifications can "catch up". That way you
> could basically introduce this fix in trunk right now, and have it
> tested and out in a stable release in no time =)
We have added such spec breaking options in the past as you'll see if
you look at the configuration docs for system properties. Generally, I'm
not a fan of configuration via system property. It is usually too blunt
a tool and doesn't provide the per webapp control that most users need.
> On Thursday, March 10, 2016 1:24 PM, [email protected] wrote:
>>
>> The issue is in ScopedAttributeELResolver.
>>
>> ScopedAttributeELResolver checks the
>> page/request/session/application context first and only if it
>> doesn't find the attribute there does it try loading the class.
>
> When debugging this in my IDE, I could see this in action. I also
> noticed that the ImportHandler that performs the class lookup is
> fetched from the ELContext object. So if I could wrap that object, I
> could simply return null in the method getImportHandler(), thus
> disabling this functionallity and therefore solving the performance
> problem for us. But I couldn't find any way to wrap the ELContext,
> for some reason. Can it be done, using standard code or config?
No.
> Or can this "import logic" be disabled some other way?
You can do most things via reflection, including this. Personally,
reflection would be my option of last resort.
> There really should be a way for users to disable this, if the
> functionallity is not used and it just is causing problems. And, like
> I said above, that could be done without breaking the specs. And an
> alternative way to the way above, could be like I mentioned before,
> to add a configuration option that forces the class name to begin
> with a capital letter. That way ${Boolean.TRUE}, ${Integer.MAX_VALUE}
> and ${MyClass.myStaticField} would still work, while ${foo.bar} etc
> would simply be ignored. As long as the configuration option would
> default to false (ie lower case first letter is allowed, as per the
> specification) it wouldn't break the specification unless the user
> deliberately told it to (which is fine, right?).
Yes, that is fine. Breaking the spec by default is fine too as long as
there is a configuration option to make Tomcat spec compatible. There
are a few things we don't do by default because most users don't use the
feature and enabling it harms performance.
> It would be really nice to get your input on these suggestions. And
> if you don't like them, could you explain why?
Generally, the suggestions are reasonable. The control can't be as
fine-grained as you suggest but that isn't necessarily a show-stopper.
The main disadvantage that these options have is that a better solution
is available. Follow the bug report from my previous message in this
thread for details.
> If your opinion is "We
> need to stick 100% to the specification, and never ever give even the
> expert user any way to override this, ever", then I would say that
> such a view causes more harm than good. :)
I don't think you'll ever find any container implementer that takes that
view.
My first preference is to fix the root cause of the problem rather than
adding configuration to work-around it.
If the root cause can't be fixed then configuration is an option with a
strong preference for per webapp rather than global.
In this case, I believe the root cause has been fixed.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
