Re: ***UNCHECKED*** RE: Intermittent ClassNotFoundException in Jasper EL evaluation

[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