Bug 52444 - Classloading-based ServletContainerInitializer @HandlesTypes processing can result in long startup times
Classloading-based ServletContainerInitializer @HandlesTypes processing can r...
Status: RESOLVED FIXED
Product: Tomcat 7
Classification: Unclassified
Component: Catalina
unspecified
PC Mac OS X 10.4
: P2 enhancement (vote)
: ---
Assigned To: Tomcat Developers Mailing List
:
: 52549 (view as bug list)
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2012-01-09 17:11 UTC by Chris Beams
Modified: 2014-02-17 13:48 UTC (History)
2 users (show)



Attachments
Fixes a NPE on startup if classes reference class or interface which aren't in the classpath (1.62 KB, patch)
2012-02-01 00:18 UTC, Guillaume Smet
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Beams 2012-01-09 17:11:12 UTC
See 52326 for background, noting particularly the following:

"As long as SCI processing involves expensive classloading, larger applications will suffer from long startup times and thus be encouraged to "shut off" this functionality via metadata-complete='true'."

This issue, then, is intended to address classloading-based approach to @HandlesTypes processing by replacing it with something faster and generally less problematic.  ASM would be one way to get it done.
Comment 1 Mark Thomas 2012-01-09 19:01:55 UTC
No functional bug here, moving to enhancement.
Comment 2 Mark Thomas 2012-01-29 12:15:57 UTC
*** Bug 52549 has been marked as a duplicate of this bug. ***
Comment 3 Mark Thomas 2012-01-29 17:36:02 UTC
Bringing across the list of suggestions from the duplicate...

> a. if the class is an annotation, skip it
Fixed in trunk and 7.0.x and will be included in 7.0.26 onwards.

> b. if the class doesn't extend/implement any interface skip it
Interesting. Thinking about this some more, the current isAssignableFrom() test is actually broader than it needs to be since it will return true for X.class.isAssignableFrom(X.class) and there is no need to add X to the initializerClassMap in this case. Apart from that however, isAssignableFrom() is the right test and that makes things a little more complicated to implement solely using byte code due to how the code currently iterates over the JARs (traversing the class hierarchy is the tricky part). Should be doable but likely to require a fair amount of re-factoring.

> c. Look at the class hierarchy - this is actually quite easy (since
> there's only one parent) and don't load it unless it implements
> ServletContextListener
This is not correct. HandlesType specify any class or interface.

> d. if there are no Servlet initializers, don't load any classes
The code already does this.

> e. if the class needs to be loaded use a throwaway classloader
If the class must be loaded to examine it yes, but hopefully it will be possible to avoid doing this.

In summary, b) is the only remaining problem to solve. The solution looks like requiring caching all the javaClass instances and then doing the HandlesTypes processing (and then throwing away the cache).
Comment 4 Costin Leau 2012-01-29 21:37:58 UTC
Thanks for looking at this Mark. The more classes can be eliminated from loading, the better.

Cheers!
Comment 5 Pid 2012-01-29 21:51:17 UTC
> In summary, b) is the only remaining problem to solve. The solution looks like
> requiring caching all the javaClass instances and then doing the HandlesTypes
> processing (and then throwing away the cache).

I was looking at using the bcel.util.SyntheticRepository when you fixed the
annotations...
Comment 6 Mark Thomas 2012-01-29 22:05:05 UTC
(In reply to comment #5)
> I was looking at using the bcel.util.SyntheticRepository when you fixed the
> annotations...

That would work but you'd need an additional cache to save you parsing the interface hierarchy every time. I'm currently working on a custom cache. Should have something for tomorrow.
Comment 7 Mark Thomas 2012-01-29 23:23:15 UTC
I think I have a patch for this. The unit tests pass but I want to run the TCK as well before I commit anything. That'll be tomorrow at the earliest now.
Comment 8 Mark Thomas 2012-01-30 11:34:58 UTC
Unit tests and Servlet TCK pass so the fix has been committed to trunk and 7.0.x. It will be in 7.0.26 onwards.
Comment 9 Chris Beams 2012-01-30 11:46:54 UTC
Thanks, Mark.  I've updated https://jira.springsource.org/browse/SPR-8894 and https://jira.springsource.org/browse/SPR-8945 to let affected users know that 7.0.26 should take care of this issue.
Comment 10 Guillaume Smet 2012-01-31 23:05:41 UTC
Hi Mark,

As we also had this problem with our application, we have built a Tomcat from svn tip and when starting the application, we had the following stacktrace:
Caused by: java.lang.NullPointerException
	at org.apache.tomcat.util.bcel.classfile.ClassParser.<init>(ClassParser.java:72)
	at org.apache.catalina.startup.ContextConfig.populateJavaClassCache(ContextConfig.java:2132)
	at org.apache.catalina.startup.ContextConfig.populateJavaClassCache(ContextConfig.java:2123)
	at org.apache.catalina.startup.ContextConfig.checkHandlesTypes(ContextConfig.java:2058)
	at org.apache.catalina.startup.ContextConfig.processAnnotationsStream(ContextConfig.java:2015)
	at org.apache.catalina.startup.ContextConfig.processAnnotationsJar(ContextConfig.java:1904)
	at org.apache.catalina.startup.ContextConfig.processAnnotationsUrl(ContextConfig.java:1872)
	at org.apache.catalina.startup.ContextConfig.processAnnotations(ContextConfig.java:1858)

This is due to the fact that classes present in the jar files may implement interfaces not present in the classpath (in our case, it's the optional commonj support in Spring's scheduling classes which rises this problem).

We solved this problem by checking that the inputStream "is" is different from null in populateJavaClassCache(String className).

With this fix, our application starts and the initialization is faster than with 7.0.25.

HTH.

-- 
Guillaume
Comment 11 Pid 2012-01-31 23:57:21 UTC
Can you add a patch as an attachment (in diff -u format) for your change?
Comment 12 Guillaume Smet 2012-02-01 00:18:32 UTC
Created attachment 28245 [details]
Fixes a NPE on startup if classes reference class or interface which aren't in the classpath

As requested, here is the trivial patch we applied to fix the NPE we had on startup.
Comment 13 Mark Thomas 2012-02-01 09:45:55 UTC
Looking at the stack trace I reached the same conclusion as to the fix but implemented it sightly differently.

This has been fixed in trunk and 7.0.x/trunk and will be included in 7.0.26 onwards.