You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@tomcat.apache.org by ji...@svensktnaringsliv.se on 2016/03/09 19:20:19 UTC
Intermittent ClassNotFoundException in Jasper EL evaluation
Hi,
We recently upgraded Tomcat from version 7 to version 8 (8.0.32 to be precise), and we immediately noticed a deterioration in the performance. Page load time increased from about 500ms to 2500-5000 ms.
One thing we noticed when we looked at some thread dumps, is that quite often a ClassNotFoundException can be seen. I found that a bit interesting, because it didn't correspond to any ClassNotFoundException in the log file. However, when troubleshooting this I noticed that the exception is caught and ignored.
Here is an example stacktrace from one thread dump:
"http-apr-8080-exec-33" Id=282 in RUNNABLE
at java.lang.Throwable.fillInStackTrace(Native Method)
at java.lang.Throwable.fillInStackTrace(Throwable.java:783)
- locked java.lang.ClassNotFoundException@26bef633
at java.lang.Throwable.<init>(Throwable.java:287)
at java.lang.Exception.<init>(Exception.java:84)
at java.lang.ReflectiveOperationException.<init>(ReflectiveOperationException.java:75)
at java.lang.ClassNotFoundException.<init>(ClassNotFoundException.java:82)
at java.net.URLClassLoader$1.run(URLClassLoader.java:372)
at java.net.URLClassLoader$1.run(URLClassLoader.java:361)
at java.security.AccessController.doPrivileged(Native Method)
at java.net.URLClassLoader.findClass(URLClassLoader.java:360)
at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
- locked java.lang.Object@2ca91dd5
at java.lang.ClassLoader.loadClass(ClassLoader.java:411)
- locked java.lang.Object@64a0e12a
at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:308)
at java.lang.ClassLoader.loadClass(ClassLoader.java:411)
- locked java.lang.Object@1b5f6952
at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
at java.lang.Class.forName0(Native Method)
at java.lang.Class.forName(Class.java:344)
at org.apache.catalina.loader.WebappClassLoaderBase.loadClass(WebappClassLoaderBase.java:1285)
- locked org.apache.catalina.loader.WebappClassLoader@254b720a
at org.apache.catalina.loader.WebappClassLoaderBase.loadClass(WebappClassLoaderBase.java:1167)
at javax.el.ImportHandler.findClass(ImportHandler.java:195)
at javax.el.ImportHandler.resolveClass(ImportHandler.java:164)
at javax.servlet.jsp.el.ScopedAttributeELResolver.getValue(ScopedAttributeELResolver.java:62)
at org.apache.jasper.el.JasperELResolver.getValue(JasperELResolver.java:110)
at org.apache.el.parser.AstIdentifier.getValue(AstIdentifier.java:80)
at org.apache.el.parser.AstEmpty.getValue(AstEmpty.java:46)
at org.apache.el.parser.AstNot.getValue(AstNot.java:43)
at org.apache.el.ValueExpressionImpl.getValue(ValueExpressionImpl.java:184)
at org.apache.jasper.runtime.PageContextImpl.proprietaryEvaluate(PageContextImpl.java:943)
at org.apache.jsp.template.framework.common.elements.sectionText.sectionText_jsp._jspx_meth_c_005fif_005f3(sectionText_jsp.java:460)
Here is the code on line 460 in sectionText_jsp.java:
_jspx_th_c_005fif_005f3.setTest(((java.lang.Boolean) org.apache.jasper.runtime.PageContextImpl.proprietaryEvaluate("${not empty leadtext}", boolean.class, (javax.servlet.jsp.PageContext)_jspx_page_context, null)).booleanValue());
And here is the corresponding line in sectionText.jsp:
<c:if test="${not empty leadtext}">
I cannot for the life of me understand how this simple EL expression can result in a ClassNotFoundException.
Also, this problem doesn't occur just with this specific jsp file. In fact, it seems to be a different jsp file involved each time. And it doesn't happen every time. The fact is that in order for me to recreate the problem in my local environment, I had to perform quite a few page requests in the browser, and still it only happened once in a while (with different jsp file each time). So it is very intermittent, and seems to be something happening deep down in the core of Tomcat itself. I'm not claiming it is a Tomcat bug, it could very well be some erroneous configuration that we are using. But the error doesn't really seem to be related to our code base, or our third party jars.
Has anyone seen this problem before? What could be the cause of it?
Regards
/Jimi
Re: Intermittent ClassNotFoundException in Jasper EL evaluation
Posted by Mark Thomas <ma...@apache.org>.
On 10/03/2016 10:19, Mark Thomas wrote:
> On 10/03/2016 08:12, jimi.hullegard@svensktnaringsliv.se wrote:
<snip/>
>> Then surely one can look at the other implementations, and what they did to avoid this problem. But one thing off the top of my head would be to at least avoid doing that class lookup in cases where it couldn't be a static field reference (like ${name}, since there is no dot after "name", there is no reason to check if "name" is a class reference, right?).
>
> It has been a while since I looked at this. I need to remind myself why
> I thought it needed to be implemented this way. I recall looking at this
> at least once before but another look wouldn't hurt.
Having reviewed this, here is some useful background.
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.
If ScopedAttributeELResolver is asked to resolve "foo" it can't
differentiate between ${ foo } which doesn't require a class lookup and
${ foo.bar } which does require a class lookup. Therefore
ScopedAttributeELResolver will always do a class lookup if it didn't
find an a matching attribute.
These lookups pass through various standard APIs (EL and JSP specs) so
we can't simply add a flag to the method to indicate if a class lookup
is required or not.
We can't use ThreadLocals because either:
- the ThreadLocal would need to be defined in a specification class
which isn't allowed because we can't change the public API; or
- the ThreadLocal would need to be defined in Tomcat and referenced in
a spec class which isn't allowed because the spec JARs have to be
independent.
However, all is not lost. I think I have a solution based on ELContext
getContext()/putContext(). Keep an eye on
https://bz.apache.org/bugzilla/show_bug.cgi?id=57583 for progress reports.
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Intermittent ClassNotFoundException in Jasper EL evaluation
Posted by Mark Thomas <ma...@apache.org>.
On 10/03/2016 22:16, Christopher Schultz wrote:
> Mark,
>
> On 3/10/16 4:43 PM, Mark Thomas wrote:
>> On 10/03/2016 21:16, jimi.hullegard@svensktnaringsliv.se wrote:
>>> On Thursday, March 10, 2016 11:20 AM, markt@apache.org 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.
>
> What about marking the ImportHandler as "dirty" and flushing a cache
> of prior lookups? (Or are we talking about spec-defined classes only,
> here?)
We are talking about spec defined classes here.
Also, ELContext and ImportHandler are not cached across requests and a
class level cache doesn't work because each page can have different imports.
I looked at caching when this issue first arose and we already cache
everything we can. Fixing the performance issue required a different
approach based on making the affected Resolver more aware of the context
in which it was being used so it could skip the ImportHandler in the
affected cases.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Intermittent ClassNotFoundException in Jasper EL evaluation
Posted by Christopher Schultz <ch...@christopherschultz.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Mark,
On 3/10/16 4:43 PM, Mark Thomas wrote:
> On 10/03/2016 21:16, jimi.hullegard@svensktnaringsliv.se wrote:
>> On Thursday, March 10, 2016 11:20 AM, markt@apache.org 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.
What about marking the ImportHandler as "dirty" and flushing a cache
of prior lookups? (Or are we talking about spec-defined classes only,
here?)
- -chris
-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - http://gpgtools.org
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlbh8j4ACgkQ9CaO5/Lv0PDP1gCgtBnEJJA9er6w5mC1MUCLofA+
qqYAn2tBNbTYYL+fuV1rIEOklNlPOfsG
=vsTe
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: ***UNCHECKED*** RE: Intermittent ClassNotFoundException in Jasper
EL evaluation
Posted by Mark Thomas <ma...@apache.org>.
On 11/03/2016 19:00, jimi.hullegard@svensktnaringsliv.se wrote:
> On Friday, March 11, 2016 6:07 PM, markt@apache.org wrote:
<snip/>
> I'm wasn't talking about gathering information regarding performance. I was talking about gathering information about what jsp/tag code and what EL variable names are responsible for this, and how often it happens. In order to get a grasp of if it is feasible to solve by fixing the code. For example, if this log output showed me that 90% of the ClassNotFoundExceptions happen because of a handful of jsp/tag files, then I would just fix these and see how much that helped. But if the log output would indicate that this problem is spread out over too many jsp and tag files, then I wouldn't even try and fix it in the code, but would instead try some other approach (like disabling this feature, or reverting back to Tomcat 7).
Ah, now I understand your motivation. debug logging would be more
helpful than a profiler in that case.
The next problem is that we are in a spec class and can't add any
dependencies. That forces a hard dependency on java.util.logging which
isn't great.
What you can do is turn on debug logging for the web application class
loader. That should get you most of what you need.
>> At the point where you'd need to access the ServletContext to read the configuration, the ServletContext isn't available.
>> [...]
>> Since I wrote the "configuration API" above, I did some more research and found the route
>> via the ELContext. That is what the implemented solution uses.
>
> So, just to check if I understood you correctly... You *can* get access to the ServletContext, right? (Using page.getServletContext()...)
No, the ServletContext isn't directly accessible but more digging shows
some helpful signs.
ScopedAttributeELResolver can access the current JspContext. That has
access to the page/request/session/application attributes and can
probably be cast to PageContext which has access to the ServletContext.
So there is a (slightly contrived) route by which per web application
config can be passed without breaking the spec and per page config can
be passed with custom code or extending the spec (which as far as the
spec owners are concerned is the same thing as breaking the spec).
>> And why would it not work for some users? I'm not saying it would magically solve the problem for everyone. But it would solve the worst problems for most people, and it wouldn't make it worse for anyone (ie, the fact that one would have the *option* to turn it off, doesn't make the situation worse).
>
>
>> Which is my point. The solution doesn't help those users that need imports and
>> good performance. Greater granularity in configuration helps to a point but
>> there will be users that need imports and good performance on the same page
>> and per expression configuration is taking things a bit too far for my taste.
>
> I'm sorry, but I don't follow you now. Are you saying that since there are people who wouldn't benefit from this (but not get hurt either), there is no reason to fix it for those that *would* benefit from it?
I'm saying that I prefer to expend effort on a solution that doesn't
break the spec and works for everyone than one that only works for some
users, probably involves breaking the spec and increases configuration
complexity.
>> Per page is certainly better than global but I still have concerns. In addition to the large pages
>> needing imports and performance above, I'm concerned that there is no good default for
>> an option that disables import support. Disabling imports by default will break pages that
>> use imports unless the developer adds the extra code. Not disabling them by default means
>> having to add code to every page with a performance problem.
>
> Well, I never said that it would solve the problem for everyone. But for people who don't use any EL imports at all (like everyone migrating from Tomcat 7), they would benefit from a global *off* option, and then they can systematically enable this feature slowly, page by page, as they add functionality that depends on it.
Page by page config requires breaking the spec which at this point I'd
rather not do by default.
> As for people who have pages that need both imports and performance, I still claim that this fix wouldn't make it any *worse* for them. In fact, I would claim that it would make it somewhat better, because now they would have an option to control what pages should have this feature, and if they have important pages that doesn't use imports then they at least could reduce the performance problem for those pages. And regarding the pages with both a need for both imports and performance, well they still have the workaround that was mentioned in the beginning of this thread, adding a specific scope to all the EL variables (at least the ones that can be null or not set).
>
> In my ears, it sounds almost like you think that having an extra option would be *bad* for some users, and I can't quite understand that reasoning. :)
No new options and a fix that works for everyone is better than a new
option that works for some people, breaks the spec by default and
requires careful analysis to get the right configuration. I'm not
completely against adding a configuration option or options if that
proves necessary but I do see downsides to doing so.
>> My preference remains addressing the root cause of the performance issue rather than trying to
>> add configuration option to optionally disable a spec feature because there is a performance issue.
>
> Yes, of course. All my thoughts and ideas about configuration options to disable the import feature completely or partly are all based on the assumption that the main problem isn't solved. If the main problem is solved, then all is good. :)
At this point I think your energies are being focussed in the wrong
direction. It is useful to have a high level idea of what the options
are since that helps make the trade-off between the benefits of further
work on the root cause and the increasing complexity of those solutions
compared to the configuration approach. At the moment, there looks to be
plenty of scope for further work to improve the root cause hence that is
where I'd prefer to be spending my time.
>>> I would also be interested in hearing your thoughts about having an option to disable
>>> this class lookup for all names that doesn't start with a capital letter.
>>
>> Such an approach would be fairly low down my list of preferred options.
>> Mainly because it depends on something outside of the developer's control.
>> If some third-party library opts not to follow the convention for
>> whatever reason (and I have seen this happen), the fix breaks.
>> The developer could work around it but it would get messy.
>
> Well, I have the same pragmatic view on this as above. It is an added option, the developer doesn't have to use it. There can be cases where the developer knows that such a problem can't happen, simply because no 3rd party jsp or tag code is used in the project. Or the developer might not know it, but performs enough testing to conclude that "all that important on the site is still working, and now the performance is great!". It is for these people the fix is for. If someone is uncertain about the implications, or actually notice big problems after enabling the feature, then they can simply ignore/remove that configuration option.
>
> In terms of documentation, you could simply state that this configuration is intended for "Experts", and that it can cause the problems you mention.
> I mean... come on. There must be loads of advanced configuration options in Tomcat that can cause problems if they are set without really knowing what one is doing. Right? :)
There are plenty of simple options that cause confusion. I'd prefer to
see fewer configuration options, not more - even if I have added a fair
few of the options over the years.
>> I've been doing some more digging. In a typical JSP page there are 4 failed lookups, each taking ~0.25ms (on my machine).
>
> Well, the estimated number of failures for a typical jsp may be interesting to know, but it is the combination all jsp pages (and tag pages) for a request that is the more interesting thing to consider. I think that many sites use quite a few different jsp and tag files, and they in turn include each other, so that a single request can result in hundreds or even thousands of jsps/tags being called. The code for our site certainly works this way. Like I said, a single request to the start page triggered 2700 CNFEs in my local machine.
Sorry I meant 4 lookups per attribute per page, not 4 look ups per page.
>
>> Even in a working case (Long.MAX_VALUE) there are still 3 failed lookups.
>
> Interesting. It got me thinking... I just realized that this feature (or the implementation of it, actually) in a way breaks the general java coding convention of not using exceptions for cases that doesn't really correspond to an *exception* to the normal flow. I'm not saying that there is a way around that, I just find it a bit interesting. Maybe the fault is with the Java API, since they decided to have the ClassLoader throw a CNFE instead of returning null. ;)
Tell me about it. This isn't the first time CNFE have caused performance
issues. The getResource() trick ImportHandler now uses has been useful
elsewhere in the Tomcat codebase.
>> we can't just take the first match because we need to check for conflicts
>
> Should this really be happening in production, if the code has been tested in development, test and stage environments? Maybe one could handle this like an assertion, i.e. only doing the collision check unless in "production mode". That would give the correct error when developing and testing, but would assume that it "can't" happen in production (just like assertions) and thus increase performance. Just a thought...
That would require a(nother) spec breaking configuration option.
>> There is a trick I used in the WebappClassloader to avoid CNFEs that could work here as well.
>> I've committed that fix as well. My very rough performance testing indicates ${foo.bar}
>> is still slower than ${foo}. ${foo.bar} looks to add somewhere between 0.1ms and 0.2ms.
>> A lot better than the 1ms it was but also still slower than ${foo}.
>
> Interesting indeed! But, I tried to look at that commit but couldn't find it. Is it not in trunk?
First attempt: http://svn.apache.org/viewvc?rev=1734592&view=rev
Fix error: http://svn.apache.org/viewvc?rev=1734594&view=rev
Improve: http://svn.apache.org/viewvc?rev=1734597&view=rev
Those are for trunk (9.0.x) but the 8.0.x ones were a few seconds
afterwards.
If you follow along in GitHub then there can be a delay before commits
are replicated from svn to GitHub.
>> I have some more ideas for further improvements but I don't want to add unnecessary complexity.
>> It would be good to get some idea of how well the current code works with your app before going further.
>> It means building from source but with 8.0.x that should be fairly painless.
>
> I can definitely do that. But now it is Friday evening here, and I'm going home. :)
> Hopefully I have time to test this on Monday.
Monday works. I might try experimenting with some ideas between now and
then anyway.
> Now I hope you have a nice weekend, Mark!
You too.
Cheers,
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
RE: Intermittent ClassNotFoundException in Jasper EL evaluation
Posted by ji...@svensktnaringsliv.se.
On Friday, March 11, 2016 6:07 PM, markt@apache.org wrote:
>
> And a debug log message is unlikely to tell you any more than the thread dump did.
That depends on what is actually being logged. If the class name is printed, then one could immediately figure out the name of the EL variable (like "java.lang.myTempValue", then the EL parameter name is "myTempValue").
And the thread dump only gives me a snapshot, containing 0, 1 or more stacktraces with the ClassNotFoundException. Unless I take a threaddump every millisecond (or even more often), the thread dump method would miss some stacktraces. With logging, it would not miss any.
> If the same code keeps appearing in a thread dump it is a fair bet that time is being spent in that code but you still need performance data - which you can't get from debug logging or thread dumps - to be certain.
I'm wasn't talking about gathering information regarding performance. I was talking about gathering information about what jsp/tag code and what EL variable names are responsible for this, and how often it happens. In order to get a grasp of if it is feasible to solve by fixing the code. For example, if this log output showed me that 90% of the ClassNotFoundExceptions happen because of a handful of jsp/tag files, then I would just fix these and see how much that helped. But if the log output would indicate that this problem is spread out over too many jsp and tag files, then I wouldn't even try and fix it in the code, but would instead try some other approach (like disabling this feature, or reverting back to Tomcat 7).
> As for using it in production, you aren't going to be using a profiler unless you have a problem.
> I'd look at the risk (never seen an issue) of using a profiler against the production system and
> the quality of data / time to useful results vs debug logging and the longer to get results / lower
> quality of data and choose the profiler every time. YMMV.
Well, I guess you have a point. But the fact is that we don't have any profiler tool installed in production right now (and I suspect that is a quite common scenario for many users in general), and having one installed is not something that is done quickly. Enabling trace or debug logging in a specific class takes less than a minute, giving me much needed information more or less instantly.
But yes, if one would have followed common sense, and already had a profiler installed (and have used it enough to know how to get it to output the needed information), then you would be right ;)
> At the point where you'd need to access the ServletContext to read the configuration, the ServletContext isn't available.
> [...]
> Since I wrote the "configuration API" above, I did some more research and found the route
> via the ELContext. That is what the implemented solution uses.
So, just to check if I understood you correctly... You *can* get access to the ServletContext, right? (Using page.getServletContext()...)
> It should be possible to hook up your proposed configuration option
> to pass data via that as long as the code is careful.
OK. Sounds promising. Fingers crossed. :)
> And why would it not work for some users? I'm not saying it would magically solve the problem for everyone. But it would solve the worst problems for most people, and it wouldn't make it worse for anyone (ie, the fact that one would have the *option* to turn it off, doesn't make the situation worse).
> Which is my point. The solution doesn't help those users that need imports and
> good performance. Greater granularity in configuration helps to a point but
> there will be users that need imports and good performance on the same page
> and per expression configuration is taking things a bit too far for my taste.
I'm sorry, but I don't follow you now. Are you saying that since there are people who wouldn't benefit from this (but not get hurt either), there is no reason to fix it for those that *would* benefit from it?
> Per page is certainly better than global but I still have concerns. In addition to the large pages
> needing imports and performance above, I'm concerned that there is no good default for
> an option that disables import support. Disabling imports by default will break pages that
> use imports unless the developer adds the extra code. Not disabling them by default means
> having to add code to every page with a performance problem.
Well, I never said that it would solve the problem for everyone. But for people who don't use any EL imports at all (like everyone migrating from Tomcat 7), they would benefit from a global *off* option, and then they can systematically enable this feature slowly, page by page, as they add functionality that depends on it.
As for people who have pages that need both imports and performance, I still claim that this fix wouldn't make it any *worse* for them. In fact, I would claim that it would make it somewhat better, because now they would have an option to control what pages should have this feature, and if they have important pages that doesn't use imports then they at least could reduce the performance problem for those pages. And regarding the pages with both a need for both imports and performance, well they still have the workaround that was mentioned in the beginning of this thread, adding a specific scope to all the EL variables (at least the ones that can be null or not set).
In my ears, it sounds almost like you think that having an extra option would be *bad* for some users, and I can't quite understand that reasoning. :)
> My preference remains addressing the root cause of the performance issue rather than trying to
> add configuration option to optionally disable a spec feature because there is a performance issue.
Yes, of course. All my thoughts and ideas about configuration options to disable the import feature completely or partly are all based on the assumption that the main problem isn't solved. If the main problem is solved, then all is good. :)
>> I would also be interested in hearing your thoughts about having an option to disable
>> this class lookup for all names that doesn't start with a capital letter.
>
> Such an approach would be fairly low down my list of preferred options.
> Mainly because it depends on something outside of the developer's control.
> If some third-party library opts not to follow the convention for
> whatever reason (and I have seen this happen), the fix breaks.
> The developer could work around it but it would get messy.
Well, I have the same pragmatic view on this as above. It is an added option, the developer doesn't have to use it. There can be cases where the developer knows that such a problem can't happen, simply because no 3rd party jsp or tag code is used in the project. Or the developer might not know it, but performs enough testing to conclude that "all that important on the site is still working, and now the performance is great!". It is for these people the fix is for. If someone is uncertain about the implications, or actually notice big problems after enabling the feature, then they can simply ignore/remove that configuration option.
In terms of documentation, you could simply state that this configuration is intended for "Experts", and that it can cause the problems you mention.
I mean... come on. There must be loads of advanced configuration options in Tomcat that can cause problems if they are set without really knowing what one is doing. Right? :)
> I've been doing some more digging. In a typical JSP page there are 4 failed lookups, each taking ~0.25ms (on my machine).
Well, the estimated number of failures for a typical jsp may be interesting to know, but it is the combination all jsp pages (and tag pages) for a request that is the more interesting thing to consider. I think that many sites use quite a few different jsp and tag files, and they in turn include each other, so that a single request can result in hundreds or even thousands of jsps/tags being called. The code for our site certainly works this way. Like I said, a single request to the start page triggered 2700 CNFEs in my local machine.
> Even in a working case (Long.MAX_VALUE) there are still 3 failed lookups.
Interesting. It got me thinking... I just realized that this feature (or the implementation of it, actually) in a way breaks the general java coding convention of not using exceptions for cases that doesn't really correspond to an *exception* to the normal flow. I'm not saying that there is a way around that, I just find it a bit interesting. Maybe the fault is with the Java API, since they decided to have the ClassLoader throw a CNFE instead of returning null. ;)
>we can't just take the first match because we need to check for conflicts
Should this really be happening in production, if the code has been tested in development, test and stage environments? Maybe one could handle this like an assertion, i.e. only doing the collision check unless in "production mode". That would give the correct error when developing and testing, but would assume that it "can't" happen in production (just like assertions) and thus increase performance. Just a thought...
> There is a trick I used in the WebappClassloader to avoid CNFEs that could work here as well.
> I've committed that fix as well. My very rough performance testing indicates ${foo.bar}
> is still slower than ${foo}. ${foo.bar} looks to add somewhere between 0.1ms and 0.2ms.
> A lot better than the 1ms it was but also still slower than ${foo}.
Interesting indeed! But, I tried to look at that commit but couldn't find it. Is it not in trunk?
> I have some more ideas for further improvements but I don't want to add unnecessary complexity.
> It would be good to get some idea of how well the current code works with your app before going further.
> It means building from source but with 8.0.x that should be fairly painless.
I can definitely do that. But now it is Friday evening here, and I'm going home. :)
Hopefully I have time to test this on Monday.
> Not at all. Civilised debate is a good thing. We don't have to agree with each other in order to learn
> something from each other's point of view or to get ideas on how to move things forward.
> Having this discussion has sparked a number of ideas for improving the code which
> I've already implemented and that can only be a good thing.
I couldn't agree more. :)
Now I hope you have a nice weekend, Mark!
Regards
/Jimi
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Intermittent ClassNotFoundException in Jasper EL evaluation
Posted by Mark Thomas <ma...@apache.org>.
On 11/03/2016 14:17, jimi.hullegard@svensktnaringsliv.se wrote:
> On Thursday, March 10, 2016 10:44 PM, markt@apache.org wrote:
>>
>> 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.
>
> Well, first of all, I found this without knowing where to look. I just checked the thread dump, looked closer at the stack traces involving the ClassNotFoundExceptions, and found the try catch clause. If there would have been a log.debug(...) or log.trace(...) statement there, then I would know how to enable debug logging for this. All without knowing beforehand where to look, and all without anybody telling me where to look.
And a debug log message is unlikely to tell you any more than the thread
dump did. If the same code keeps appearing in a thread dump it is a fair
bet that time is being spent in that code but you still need performance
data - which you can't get from debug logging or thread dumps - to be
certain.
> Secondly, not all problems are easily reproducible in a development/test/staging environment, especially if one depends on the nature of the production traffic to find all (or at least most) places in the code that experience problems. Not all organizations have the time and/or the money to replay/simulate live traffic in another environment. And I suspect that most IT departments and/or service providers would react with some suspicion or worry if one would try and get them to introduce a profiler in production, even though these tools have become more and more production friendly over the years.
You still need to know what debug logging to turn on and if you are
guessing based on thread dumps then you already have everything debug
logging is likely to give you.
> Thirdly, even if profiling would be an option, if no profiling is setup already then it would take some time and money to do that. Compared to the trivial change of logging level for a specific class. And I can't really see how log.debug(...) or log.trace(...) (possibly surrounded by a log.isDebugEnabled() or isTraceEnabled())would have any negative effect for anyone.
Compared to the cost of the time to debug issues without a profiler, the
cost of a profiler is tiny. I use YourKit (because they give me a free
copy to use for my ASF work) and, while the price of it has gone up
quite a bit since I last looked a few years ago, it still (in my view)
offers a very quick return on investment.
As for using it in production, you aren't going to be using a profiler
unless you have a problem. I'd look at the risk (never seen an issue) of
using a profiler against the production system and the quality of data /
time to useful results vs debug logging and the longer to get results /
lower quality of data and choose the profiler every time. YMMV.
>> 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.
>
> I'm not sure I understand why the absence of a "configuration API" would stop you from simply checking for a specific init parameter in the servlet context (thus making it possible to configure it in the web.xml using a context-param). And when it comes to per jsp configuration, one could check for a page scoped attribute with the same name. Ideally this could be added as a new page directive attribute (like <%@ page disableELImports="true" %>), but I guess that would constitute a "configuration API" change that would have to be in the specification. But the init parameter or page scoped attribute could be fine interim solutions, until the specification is updated to add these configuration options the proper way.
At the point where you'd need to access the ServletContext to read the
configuration, the ServletContext isn't available. The code in question
is in a spec JAR and we can't change the public API in any way, nor can
we add a dependency to some other component. That tends to leave system
properties as the only option.
Since I wrote the "configuration API" above, I did some more research
and found the route via the ELContext. That is what the implemented
solution uses. It should be possible to hook up your proposed
configuration option to pass data via that as long as the code is careful.
> And why would it not work for some users? I'm not saying it would magically solve the problem for everyone. But it would solve the worst problems for most people, and it wouldn't make it worse for anyone (ie, the fact that one would have the *option* to turn it off, doesn't make the situation worse).
Which is my point. The solution doesn't help those users that need
imports and good performance. Greater granularity in configuration helps
to a point but there will be users that need imports and good
performance on the same page and per expression configuration is taking
things a bit too far for my taste.
>> Generally, the suggestions are reasonable. The control can't be as
>> fine-grained as you suggest but that isn't necessarily a show-stopper.
>
> Well, the solution with the page scoped attribute in the jsp page would make it fine grained enough for most people, don't you think? :)
Per page is certainly better than global but I still have concerns. In
addition to the large pages needing imports and performance above, I'm
concerned that there is no good default for an option that disables
import support. Disabling imports by default will break pages that use
imports unless the developer adds the extra code. Not disabling them by
default means having to add code to every page with a performance problem.
My preference remains addressing the root cause of the performance issue
rather than trying to add configuration option to optionally disable a
spec feature because there is a performance issue.
> I would also be interested in hearing your thoughts about having an option to disable this class lookup for all names that doesn't start with a capital letter. Thus making ${Boolean.TRUE} work as normal, while skipping ${foo.bar}, even if they coexist in the same jsp page. I actually would think that would be the best solution to this, since the naming convention dictates that you should start class names with capital letter, and variables with lower case letters.
Such an approach would be fairly low down my list of preferred options.
Mainly because it depends on something outside of the developer's
control. If some third-party library opts not to follow the convention
for whatever reason (and I have seen this happen), the fix breaks. The
developer could work around it but it would get messy.
>> 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.
>> [...]
>> In this case, I believe the root cause has been fixed.
>
> That sounds great! And I saw now in the bug report page that your simple test case indicated a ~1ms performance gain per avoided ClassNotFoundException. That sounds very close to my own tests, where the start page takes about 2500 ms, and I counted about 2700 ClassNotFoundExceptions.
I've been doing some more digging. In a typical JSP page there are 4
failed lookups, each taking ~0.25ms (on my machine). Even in a working
case (Long.MAX_VALUE) there are still 3 failed lookups. Note not all of
that 0.25ms is caused by the CNFE but a large chunk of it is.
> But, just a question. When looking at the code change in commit 1734419, it seems to only handle the special case of ${foo} or ${empty foo} and similar. But the problem would still remain for ${foo.bar}, right? So, technically, the root cause is not fixed completely. We have lots of ${foo.bar} equivalents in our code, where it is not uncommon for "foo" to be null. So I suspect that we still would experience a big performance degradation on our sites, even with this fix, compared to simply using Tomcat 7.
Correct. The EL processing has no way (ignoring naming conventions) to
differentiate between ${ foo.bar } and ${ Long.MAX_VALUE }.
/me pauses to think
The bulk of the delay is caused by the CNFEs which in turn are caused by
having to look up the identifier in all of the imported packages (we
can't just take the first match because we need to check for conflicts).
There is a trick I used in the WebappClassloader to avoid CNFEs that
could work here as well.
I've committed that fix as well. My very rough performance testing
indicates ${foo.bar} is still slower than ${foo}. ${foo.bar} looks to
add somewhere between 0.1ms and 0.2ms. A lot better than the 1ms it was
but also still slower than ${foo}.
Since it is still faster to avoid the ImportHandler completely if
possible so I'll leave the previous fix in place.
I have some more ideas for further improvements but I don't want to add
unnecessary complexity. It would be good to get some idea of how well
the current code works with your app before going further.
It means building from source but with 8.0.x that should be fairly painless.
> /Jimi
>
> P.S. Having said all this. I hope you don't think I sound like an overcritical fault-finding bastard. I just find it interesting do discuss this. :) And a solution to this problem would mean that we didn't have to revert back to Tomcat 7 for our sites. D.S.
Not at all. Civilised debate is a good thing. We don't have to agree
with each other in order to learn something from each other's point of
view or to get ideas on how to move things forward. Having this
discussion has sparked a number of ideas for improving the code which
I've already implemented and that can only be a good thing.
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
RE: Intermittent ClassNotFoundException in Jasper EL evaluation
Posted by ji...@svensktnaringsliv.se.
On Thursday, March 10, 2016 10:44 PM, markt@apache.org wrote:
>
> 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.
Well, first of all, I found this without knowing where to look. I just checked the thread dump, looked closer at the stack traces involving the ClassNotFoundExceptions, and found the try catch clause. If there would have been a log.debug(...) or log.trace(...) statement there, then I would know how to enable debug logging for this. All without knowing beforehand where to look, and all without anybody telling me where to look.
Secondly, not all problems are easily reproducible in a development/test/staging environment, especially if one depends on the nature of the production traffic to find all (or at least most) places in the code that experience problems. Not all organizations have the time and/or the money to replay/simulate live traffic in another environment. And I suspect that most IT departments and/or service providers would react with some suspicion or worry if one would try and get them to introduce a profiler in production, even though these tools have become more and more production friendly over the years.
Thirdly, even if profiling would be an option, if no profiling is setup already then it would take some time and money to do that. Compared to the trivial change of logging level for a specific class. And I can't really see how log.debug(...) or log.trace(...) (possibly surrounded by a log.isDebugEnabled() or isTraceEnabled())would have any negative effect for anyone.
Just my two cents...
> 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.
I'm not sure I understand why the absence of a "configuration API" would stop you from simply checking for a specific init parameter in the servlet context (thus making it possible to configure it in the web.xml using a context-param). And when it comes to per jsp configuration, one could check for a page scoped attribute with the same name. Ideally this could be added as a new page directive attribute (like <%@ page disableELImports="true" %>), but I guess that would constitute a "configuration API" change that would have to be in the specification. But the init parameter or page scoped attribute could be fine interim solutions, until the specification is updated to add these configuration options the proper way.
> Um, no. See above. This would have to be a global option. It would work for some users/pages but not for others.
Um, yes, see above. ;)
And why would it not work for some users? I'm not saying it would magically solve the problem for everyone. But it would solve the worst problems for most people, and it wouldn't make it worse for anyone (ie, the fact that one would have the *option* to turn it off, doesn't make the situation worse).
> Generally, the suggestions are reasonable. The control can't be as
> fine-grained as you suggest but that isn't necessarily a show-stopper.
Well, the solution with the page scoped attribute in the jsp page would make it fine grained enough for most people, don't you think? :)
I would also be interested in hearing your thoughts about having an option to disable this class lookup for all names that doesn't start with a capital letter. Thus making ${Boolean.TRUE} work as normal, while skipping ${foo.bar}, even if they coexist in the same jsp page. I actually would think that would be the best solution to this, since the naming convention dictates that you should start class names with capital letter, and variables with lower case letters.
Basically, the code could look something like this, in ScopedAttributeELResolver.java:
if (importHandler != null) {
if (resolveClass) {
//disableELImportsForNamesNotStartingWithUpperCase is false by default,
//and can be configured using for example a web.xml init parameter
if (disableELImportsForNamesNotStartingWithUpperCase && !Character.isUpperCase(key.charAt(0))) {
resolveClass = false;
}
}
if (resolveClass) {
...
}
...
> 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.
> [...]
> In this case, I believe the root cause has been fixed.
That sounds great! And I saw now in the bug report page that your simple test case indicated a ~1ms performance gain per avoided ClassNotFoundException. That sounds very close to my own tests, where the start page takes about 2500 ms, and I counted about 2700 ClassNotFoundExceptions.
But, just a question. When looking at the code change in commit 1734419, it seems to only handle the special case of ${foo} or ${empty foo} and similar. But the problem would still remain for ${foo.bar}, right? So, technically, the root cause is not fixed completely. We have lots of ${foo.bar} equivalents in our code, where it is not uncommon for "foo" to be null. So I suspect that we still would experience a big performance degradation on our sites, even with this fix, compared to simply using Tomcat 7.
/Jimi
P.S. Having said all this. I hope you don't think I sound like an overcritical fault-finding bastard. I just find it interesting do discuss this. :) And a solution to this problem would mean that we didn't have to revert back to Tomcat 7 for our sites. D.S.
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Intermittent ClassNotFoundException in Jasper EL evaluation
Posted by Mark Thomas <ma...@apache.org>.
On 10/03/2016 21:16, jimi.hullegard@svensktnaringsliv.se wrote:
> On Thursday, March 10, 2016 11:20 AM, markt@apache.org 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, markt@apache.org 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: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
RE: Intermittent ClassNotFoundException in Jasper EL evaluation
Posted by ji...@svensktnaringsliv.se.
On Thursday, March 10, 2016 11:20 AM, markt@apache.org 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).
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?
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?
> > 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.
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? 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! =)
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 =)
On Thursday, March 10, 2016 1:24 PM, markt@apache.org 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? Or can this "import logic" be disabled some other way?
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?).
It would be really nice to get your input on these suggestions. And if you don't like them, could you explain why? 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. :)
Regards
/Jimi
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Intermittent ClassNotFoundException in Jasper EL evaluation
Posted by Mark Thomas <ma...@apache.org>.
On 10/03/2016 08:12, jimi.hullegard@svensktnaringsliv.se wrote:
> On Wednesday, March 9, 2016 8:22 PM, markt@apache.org wrote:
>> It is a known 'feature' of the new EL requirements added in 3.0. The EL parser can't differentiate
>> between an attribute without a scope and a reference to an static field.
>>
>> See https://bz.apache.org/bugzilla/show_bug.cgi?id=57583
>
> Interesting. I can see how that in a way could explain the the performance regression. But what I don't understand is:
>
> 1. Why would this cause a ClassNotFoundException? What class exactly is it trying to load?
For each imported package, it is trying to load package.attributeName
> 2. Why is this happening seemingly intermittently, with different EL variables each time?
It isn't. It is happening every time. You are just observing it
intermittently. Probably because class loading is serial by default and
how observable this is depends on how many threads are trying to load
classes in parallel. Note that 9.0.x will use ParallelWebappClassLoader
by default from the next release and you can always switch to that class
loader in 8.0.x
> 3. Why is the problem not limited to the first request for a jsp page? We see this problem even days after a restart, for jsp-pages that definitely have been used multiple times already, with the same state for it's variables.
Because EL imports may be dynamic so the EL has to be evaluated on
execution.
> 4. Why isn't the ClassNotFoundException logged by the ImportHandler?
Because it is expected and logging it provides no value to the user.
> 5. Why would this change between Tomcat 7 and Tomcat 8, with the exact same webapp war with the exact same web.xml?
Because imports and statics were added in EL 3.0 which was first
implemented in Tomcat 8.
> 6. In our web.xml we specify version="2.5". Wouldn't that mean EL version 2.1? (http://tomcat.apache.org/whichversion.html)
No. The version you specify in web.xml only determines the rules that
Tomcat uses to validate the web.xml. It has zero impact on the behaviour
of the container. The overall JavaEE EG made it very clear that this is
how they expected implementations to behave.
>> The way to avoid it is to always add an explicit scope (page, request, application, session) to your attributes.
>
> Is this an official recommendation, stemming from the EL 3.0 specifications? If so, can you point me to that paragraph in the specification document, or some other paper of similar nature? Because when I look at the 3.0 specification document, all I see is examples without scope.
>
> Or is this just a pragmatic response to the Tomcat/Jasper implementation of the specifications?
It is the best recommendation I have right now based on what I know
about the EL spec and Tomcat's implementation of it.
> The reason I ask is that a simple search in our code base show that we have about 10.000 potential candidates for the change you are suggesting, spread out in hundreds of jsp files in our project. And on top of that, the CMS that we use have their own jsp files, and a quick check indicates thousands of potential candidates for the change there as well. So not only would we have to perform a monumental task in our own code (because we would need to determine the scope manually, for each and every variable), we also would need to ask the CMS company to perform the same task.
You aren't the only one in that position.
>> Suggestions for improvements to the default ImportHandler implementation to make this faster are welcome.
>
> Well, I am quite pragmatic in my thinking. Is this EL implementation the only implementation with this problem?
I don't know. There aren't that many JSP implementations out there. The
end result should be the same with all of them but how they get there
may vary.
> Then surely one can look at the other implementations, and what they did to avoid this problem. But one thing off the top of my head would be to at least avoid doing that class lookup in cases where it couldn't be a static field reference (like ${name}, since there is no dot after "name", there is no reason to check if "name" is a class reference, right?).
It has been a while since I looked at this. I need to remind myself why
I thought it needed to be implemented this way. I recall looking at this
at least once before but another look wouldn't hurt.
> Otherwise, if more or less all implementations suffer from this problem, then maybe it is the specification that is to blame. Maybe, when introducing the new concept of EL references to static fields, they could use a special prefix, like ${static.Boolean.True}, or they could have had this feature turned off by default, with the possibility to turn it on either per jsp page (using some page directive like <%@ page staticElReferences="true" %>) or webapp-globally in the web.xml. Or, they could simply include the requirement that the class name must start with a capital letter, thus only causing problems for people who break the coding standard (ie either having a class name starting with a lower case letter, or having a variable name starting with an upper case letter).
The spec process could be better. I was on the EL EG but as far as I am
aware none of the implementations received any significant use before
the spec was finalized so I'm not surprised that issues like this
appeared as adoption grew.
The JavaEE specs are very big on backwards compatibility. Therefore, the
chances of changing the spec syntax to fix this are zero.
I should be able to look at this later today.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
RE: Intermittent ClassNotFoundException in Jasper EL evaluation
Posted by ji...@svensktnaringsliv.se.
On Wednesday, March 9, 2016 8:22 PM, markt@apache.org wrote:
> It is a known 'feature' of the new EL requirements added in 3.0. The EL parser can't differentiate
> between an attribute without a scope and a reference to an static field.
>
> See https://bz.apache.org/bugzilla/show_bug.cgi?id=57583
Interesting. I can see how that in a way could explain the the performance regression. But what I don't understand is:
1. Why would this cause a ClassNotFoundException? What class exactly is it trying to load?
2. Why is this happening seemingly intermittently, with different EL variables each time?
3. Why is the problem not limited to the first request for a jsp page? We see this problem even days after a restart, for jsp-pages that definitely have been used multiple times already, with the same state for it's variables.
4. Why isn't the ClassNotFoundException logged by the ImportHandler?
5. Why would this change between Tomcat 7 and Tomcat 8, with the exact same webapp war with the exact same web.xml?
6. In our web.xml we specify version="2.5". Wouldn't that mean EL version 2.1? (http://tomcat.apache.org/whichversion.html)
> The way to avoid it is to always add an explicit scope (page, request, application, session) to your attributes.
Is this an official recommendation, stemming from the EL 3.0 specifications? If so, can you point me to that paragraph in the specification document, or some other paper of similar nature? Because when I look at the 3.0 specification document, all I see is examples without scope.
Or is this just a pragmatic response to the Tomcat/Jasper implementation of the specifications?
The reason I ask is that a simple search in our code base show that we have about 10.000 potential candidates for the change you are suggesting, spread out in hundreds of jsp files in our project. And on top of that, the CMS that we use have their own jsp files, and a quick check indicates thousands of potential candidates for the change there as well. So not only would we have to perform a monumental task in our own code (because we would need to determine the scope manually, for each and every variable), we also would need to ask the CMS company to perform the same task.
> Suggestions for improvements to the default ImportHandler implementation to make this faster are welcome.
Well, I am quite pragmatic in my thinking. Is this EL implementation the only implementation with this problem? Then surely one can look at the other implementations, and what they did to avoid this problem. But one thing off the top of my head would be to atleast avoid doing that class lookup in cases where it couldn't be a static field reference (like ${name}, since there is no dot after "name", there is no reason to check if "name" is a class reference, right?).
Otherwise, if more or less all implementations suffer from this problem, then maybe it is the specification that is to blame. Maybe, when introducing the new concept of EL references to static fields, they could use a special prefix, like ${static.Boolean.True}, or they could have had this feature turned off by default, with the possibility to turn it on either per jsp page (using some page directive like <%@ page staticElReferences="true" %>) or webapp-globally in the web.xml. Or, they could simply include the requirement that the class name must start with a capital letter, thus only causing problems for people who break the coding standard (ie either having a class name starting with a lower case letter, or having a variable name starting with an upper case letter).
/Jimi
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Intermittent ClassNotFoundException in Jasper EL evaluation
Posted by Mark Thomas <ma...@apache.org>.
On 09/03/2016 18:20, jimi.hullegard@svensktnaringsliv.se wrote:
> Has anyone seen this problem before? What could be the cause of it?
It is a known 'feature' of the new EL requirements added in 3.0. The EL
parser can't differentiate between an attribute without a scope and a
reference to an static field.
See https://bz.apache.org/bugzilla/show_bug.cgi?id=57583
The way to avoid it is to always add an explicit scope (page, request,
application, session) to your attributes.
Suggestions for improvements to the default ImportHandler implementation
to make this faster are welcome.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org