Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issues with Spring LoadTimeWeaving/Class Transformation on Tomcat 8 #1171

Closed
phillipuniverse opened this issue Nov 18, 2014 · 22 comments
Closed

Comments

@phillipuniverse
Copy link
Contributor

This has been proven to happen on both Tomcat 8 and Tomcat 7.0.53. When turning on TRACE level debugging, some classes are detected as not having been transformed even though they should be.

From @ktisdell, this was the output on Tomcat 7.0.53:

[TRACE] 17:12:53 MergePersistenceUnitManager - Should have transformed org.broadleafcommerce.core.order.fulfillment.domain.FulfillmentBandImpl but didn't
[TRACE] 17:12:53 MergePersistenceUnitManager - Should have transformed org.broadleafcommerce.core.search.domain.SearchInterceptImpl but didn't
[TRACE] 17:12:53 MergePersistenceUnitManager - Should have transformed com.broadleafcommerce.tenant.domain.MultiTenantAdminRoleImpl but didn't
[TRACE] 17:12:53 MergePersistenceUnitManager - Should have transformed com.broadleafcommerce.tenant.domain.MultiTenantAdminUserImpl but didn't

For me, I had some of the same (like MultiTenantAdminUserImpl) but also some different ones (like ProductImpl):

TRACE] 01:25:00 MergePersistenceUnitManager - Should have transformed org.broadleafcommerce.common.site.domain.SiteImpl but didn't
[TRACE] 01:25:00 MergePersistenceUnitManager - Should have transformed org.broadleafcommerce.common.sitemap.domain.SiteMapGeneratorConfigurationImpl but didn't
[TRACE] 01:25:00 MergePersistenceUnitManager - Should have transformed org.broadleafcommerce.core.catalog.domain.ProductImpl but didn't
[TRACE] 01:25:00 MergePersistenceUnitManager - Should have transformed org.broadleafcommerce.core.order.fulfillment.domain.FulfillmentBandImpl but didn't
[TRACE] 01:25:00 MergePersistenceUnitManager - Should have transformed org.broadleafcommerce.core.order.domain.OrderItemImpl but didn't
[TRACE] 01:25:00 MergePersistenceUnitManager - Should have transformed org.broadleafcommerce.core.search.domain.SearchInterceptImpl but didn't
[TRACE] 01:25:00 MergePersistenceUnitManager - Should have transformed org.broadleafcommerce.cms.file.domain.StaticAssetImpl but didn't
[TRACE] 01:25:00 MergePersistenceUnitManager - Should have transformed org.broadleafcommerce.cms.url.domain.URLHandlerImpl but didn't
[TRACE] 01:25:00 MergePersistenceUnitManager - Should have transformed com.broadleafcommerce.tenant.domain.MultiTenantAdminRoleImpl but didn't
[TRACE] 01:25:00 MergePersistenceUnitManager - Should have transformed com.broadleafcommerce.tenant.domain.MultiTenantAdminUserImpl but didn't
[TRACE] 01:25:00 MergePersistenceUnitManager - Should have transformed com.broadleafcommerce.enterprise.workflow.domain.change.MapStructureImpl but didn't

First of all, if I am not mistaken this represents a very serious error state so I think that the debug logging needs to change to the ERROR level (and not TRACE).

I have also determined it is definitely later versions of Tomcat by trying these permutations:

  • Java 7 Tomcat 7 are fine
  • Java 7 bytecode with Java 8 JVM and Tomcat 7 is fine
  • Java 8 bytecode with Java 8 JVM and Tomcat 7 is fine; looks like it might just be Tomcat 8
  • Java 7 bytecode and Tomcat 8 gives me the exception

Other references from Spring issues that have been opened regarding changes to Tomcat's class loader: https://jira.spring.io/browse/SPR-11447 and https://jira.spring.io/browse/SPR-11446.

As we have now confirmed that this affects later versions of Tomcat 7 (not just Tomcat 8) then I think this is a very critical issue.

@phillipuniverse
Copy link
Contributor Author

Also, this is confirmed on Spring 4.1.2.RELEASE with spring-instrument-4.1.2-RELEASE.jar. With Tomcat 8 specifically, I also tried using spring-instrument-tomcat-4.1.2-RELEASE.jar in Tomcat's lib directory and using a special classloader. Got the same results there.

@phillipuniverse
Copy link
Contributor Author

Linking to #843 that had very similar problems. We have confirmed that class transformation is being skipped by only using -javaagent without having spring-instrument-tomcat in Tomcat's classpath.

@phillipuniverse
Copy link
Contributor Author

Need to test this on 3.1 vs 3.2 to see if this is a problem with Spring 4.

@jefffischer
Copy link
Member

btw - the error logging is currently reporting some classes that "miss" transformation incorrectly (e.g. FulfillmentBandImpl)

@phillipuniverse
Copy link
Contributor Author

Yup I'm on that. But why does it report them incorrectly? Why is it 'ok' that those classes are not transformed?

@jefffischer
Copy link
Member

No idea - just mentioned it because, for example, FulfillmentBandImpl is just a @MappedSuperClass, not an @entity. Some of the other reported are @embeddable. I'm not sure why they're making it in there - I haven't reviewed that code.

@phillipuniverse
Copy link
Contributor Author

Ok, had a pretty good debugging session and with Tomcat 8 on Broadleaf 3.1.9-SNAPSHOT, I have observed the following in regards to logging these classes:

[TRACE] 17:12:53 MergePersistenceUnitManager - Should have transformed org.broadleafcommerce.core.order.fulfillment.domain.FulfillmentBandImpl but didn't
[TRACE] 17:12:53 MergePersistenceUnitManager - Should have transformed org.broadleafcommerce.core.search.domain.SearchInterceptImpl but didn't
[TRACE] 17:12:53 MergePersistenceUnitManager - Should have transformed com.broadleafcommerce.tenant.domain.MultiTenantAdminRoleImpl but didn't
[TRACE] 17:12:53 MergePersistenceUnitManager - Should have transformed com.broadleafcommerce.tenant.domain.MultiTenantAdminUserImpl but didn't

It seems that all of them are crapping out in javaassist's loading. For instance, take these lines from DirectCopyClassTransformer (line 147):

classPool = ClassPool.getDefault();
clazz = classPool.makeClass(new ByteArrayInputStream(classfileBuffer), false);
List<?> attributes = clazz.getClassFile().getAttributes();
Iterator<?> itr = attributes.iterator();
List<String> templates = new ArrayList<String>();
List<Boolean> skips = new ArrayList<Boolean>();
List<Boolean> renames = new ArrayList<Boolean>();
check: {
    while(itr.hasNext()) {
        Object object = itr.next();
        if (AnnotationsAttribute.class.isAssignableFrom(object.getClass())) {
            AnnotationsAttribute attr = (AnnotationsAttribute) object;
            Annotation[] items = attr.getAnnotations();
...

The issue is that when we invoke:

List<?> attributes clazz.getClassFile().getAttributes();

for FulfillmentBandImpl, I get this list:
screen shot 2014-11-25 at 10 21 53 pm

Since neither of these are instances of AnnotationsAttribute, then this is skipped, which is fine because this class is looking for @DirectCopyTransform anyway. However, this is a problem because the EntityMarkerClassTransformer does the same check to put the class into the list of classes that it 'transformed'. Since this class does not have an AnnotationsAttribute attribute, the class name never gets in the list, and thus when MergedPersistenceUnit goes back over all the classes to verify then it thinks there is a problem (when there isn't).

So that's what is going on with those 4 classes specifically. However, it doesn't make any sense yet as to why that is the case. According to the Javassist docs, a class should have an AnnotationsAttribute attribute as long as they have at least 1 annotation with @Retention(RetentionPolicy.RUNTIME). These are how those classes are annotated:

/** This annotation is defined as
 * @Documented
 * @Target({TYPE})
 * @Retention(RUNTIME)
 * public @interface MappedSuperclass {
 */
@MappedSuperclass
public abstract class FulfillmentBandImpl
/** This annotation is defined as
 * @Documented
 * @Target({TYPE})
 * @Retention(RUNTIME)
 * public @interface Embeddable {
 */
@Embeddable
public class MultiTenantAdminUserImpl implements MultiTenantAdminUser {
/** This annotation is defined as
 * @Documented
 * @Target({TYPE})
 * @Retention(RUNTIME)
 * public @interface Embeddable {
 */
@Embeddable
public class MultiTenantAdminUserImpl implements MultiTenantAdminUser {
// This is an error; the only reason this is being loaded is because it is still appearing in a persistence.xml and thus still being loaded by the persistence manager
@Deprecated
@Cache(usage = CacheConcurrencyStrategy.READ_WRITE, region="blStandardElements")
public class SearchInterceptImpl implements SearchIntercept {

So Javaassist thinks that MultiTenantAdminUserImpl, MultiTenantAdminRoleImpl and FulfillmentBandImpl all do not have runtime-visible annotations. This is obviously false and I don't yet know why Javassist can't deal with these classes specifically.

@phillipuniverse
Copy link
Contributor Author

Ah, ok, I was misinterpreting the results from above. Looks like the problem is not with Javassist (surprise surprise). Even though Eclipse was printing out MappedSuperclass there for what was inside, it is actually an instance of AnnotationsAttribute.

The problem is solely in EntityMarkerClassTransformer which is only marking classes that have an @Entity annotation. This is obviously incorrect since there are perfectly valid cases with only @Embeddable and @MappedSuperclass annotations that should still get loaded by the persistence manager.

The really peculiar part of this is that this was not a much larger epidemic. I would think that this would've affected all of our @Embeddables (Auditable, Weight, etc).

To be clear, this does not fix the original problem. However, I believe we have now confirmed that this issue is not present in 3.1.9.

phillipuniverse added a commit that referenced this issue Nov 26, 2014
phillipuniverse added a commit that referenced this issue Nov 26, 2014
…nger has any `@Entity` or `@Table` annotations and is completely benign
phillipuniverse added a commit that referenced this issue Nov 26, 2014
@phillipuniverse
Copy link
Contributor Author

Moving out of 3.1 to only 3.2.

@phillipuniverse phillipuniverse modified the milestones: 3.2.0-GA, 3.1.9-GA Nov 26, 2014
@phillipuniverse
Copy link
Contributor Author

I believe that I have narrowed down this problem to EHCache. It appears that, in the background, EHCache is loading ProductImpl and thus hijacking the process by which Tomcat is loading classes and then transforming them.

This is the method within Tomcat's WebappClassLoaderBase that does the wrong thing:

    protected ResourceEntry findResourceInternal(final String name, final String path)

WebappClassLoaderBase has a list of ClassFileTransformers that findResourceInternal loops through in order to transform classes. The fact that WebappClassLoaderBase implements the InstrumentableClassLoader is a new feature of Tomcat 8. I have confirmed that for ProductImpl, Tomcat sees that it has already been loaded (I think; not 100% sure what these ResourceEntries are) because it returns early:

    protected ResourceEntry findResourceInternal(final String name, final String path) {

        if (!state.isAvailable()) {
            log.info(sm.getString("webappClassLoader.stopped", name));
            return null;
        }

        if (name == null || path == null) {
            return null;
        }
        // This is returning a non-null entry. On every other class that I observed, this returns null and it continues on to the class transformers
        ResourceEntry entry = resourceEntries.get(path);
        if (entry != null) {
            return entry;
        }
    ...
    ...
    // Remaining implementation excluded, but below here is where it loops through the configured `ClassFileTransformer`s

I suspect EhCache because my logging looks like this (after change the Tomcat logging to FINE by referencing a logging.properties from Tomcat in Eclipse: http://www.eclipse.org/forums/index.php/t/56855/):

....
....
// Finished transforming ProductAttributeImpl
[SUPPORT] 19:53:39 MultiTenantClassTransformer - MultiTenant-SingleSchema - END - Transform - Copied multi-tenant filter definitions and filters into [org.broadleafcommerce.core.catalog.domain.ProductAttributeImpl]
26-Nov-2014 19:53:39.015 FINE [localhost-startStop-1] org.apache.catalina.loader.WebappClassLoaderBase.loadClass loadClass(org.broadleafcommerce.core.catalog.domain.ProductAttribute, false)
26-Nov-2014 19:53:39.015 FINE [localhost-startStop-1] org.apache.catalina.loader.WebappClassLoaderBase.loadClass   Searching local repositories
26-Nov-2014 19:53:39.016 FINE [localhost-startStop-1] org.apache.catalina.loader.WebappClassLoaderBase.findClass     findClass(org.broadleafcommerce.core.catalog.domain.ProductAttribute)
26-Nov-2014 19:53:39.016 FINE [localhost-startStop-1] org.apache.catalina.loader.WebappClassLoaderBase.loadClass   Loading class from local repository
26-Nov-2014 19:53:39.016 FINE [localhost-startStop-1] org.apache.catalina.loader.WebappClassLoaderBase.loadClass   Loading class from local repository
26-Nov-2014 19:53:40.254 FINE [generated%0052esource%0043ache.data] org.apache.catalina.loader.WebappClassLoaderBase.loadClass loadClass(net.sf.ehcache.store.disk.DiskStore$KeySet, false)
26-Nov-2014 19:53:40.254 FINE [generated%0052esource%0043ache.data] org.apache.catalina.loader.WebappClassLoaderBase.loadClass   Searching local repositories
26-Nov-2014 19:53:40.254 FINE [generated%0052esource%0043ache.data] org.apache.catalina.loader.WebappClassLoaderBase.findClass     findClass(net.sf.ehcache.store.disk.DiskStore$KeySet)
26-Nov-2014 19:53:40.415 FINE [generated%0052esource%0043ache.data] org.apache.catalina.loader.WebappClassLoaderBase.loadClass   Loading class from local repository
26-Nov-2014 19:53:40.415 FINE [generated%0052esource%0043ache.data] org.apache.catalina.loader.WebappClassLoaderBase.loadClass loadClass(net.sf.ehcache.store.disk.DiskStore$KeyIterator, false)
26-Nov-2014 19:53:40.415 FINE [generated%0052esource%0043ache.data] org.apache.catalina.loader.WebappClassLoaderBase.loadClass   Searching local repositories
26-Nov-2014 19:53:40.415 FINE [generated%0052esource%0043ache.data] org.apache.catalina.loader.WebappClassLoaderBase.findClass     findClass(net.sf.ehcache.store.disk.DiskStore$KeyIterator)
26-Nov-2014 19:53:40.428 FINE [generated%0052esource%0043ache.data] org.apache.catalina.loader.WebappClassLoaderBase.loadClass loadClass(net.sf.ehcache.store.disk.DiskStore$HashIterator, false)
26-Nov-2014 19:53:40.428 FINE [generated%0052esource%0043ache.data] org.apache.catalina.loader.WebappClassLoaderBase.loadClass   Searching local repositories
26-Nov-2014 19:53:40.428 FINE [generated%0052esource%0043ache.data] org.apache.catalina.loader.WebappClassLoaderBase.findClass     findClass(net.sf.ehcache.store.disk.DiskStore$HashIterator)
26-Nov-2014 19:53:40.440 FINE [generated%0052esource%0043ache.data] org.apache.catalina.loader.WebappClassLoaderBase.loadClass   Loading class from local repository
26-Nov-2014 19:53:40.440 FINE [generated%0052esource%0043ache.data] org.apache.catalina.loader.WebappClassLoaderBase.loadClass   Loading class from local repository
26-Nov-2014 19:53:40.441 FINE [generated%0052esource%0043ache.data] org.apache.catalina.loader.WebappClassLoaderBase.loadClass loadClass(net.sf.ehcache.store.disk.Segment$HashIterator, false)
26-Nov-2014 19:53:40.441 FINE [generated%0052esource%0043ache.data] org.apache.catalina.loader.WebappClassLoaderBase.loadClass   Searching local repositories
26-Nov-2014 19:53:40.441 FINE [generated%0052esource%0043ache.data] org.apache.catalina.loader.WebappClassLoaderBase.findClass     findClass(net.sf.ehcache.store.disk.Segment$HashIterator)
26-Nov-2014 19:53:40.454 FINE [generated%0052esource%0043ache.data] org.apache.catalina.loader.WebappClassLoaderBase.loadClass   Loading class from local repository
26-Nov-2014 19:55:52.294 FINE [localhost-startStop-1] org.apache.catalina.loader.WebappClassLoaderBase.loadClass loadClass(org.broadleafcommerce.core.catalog.domain.ProductImpl, false)
26-Nov-2014 19:55:52.294 FINE [localhost-startStop-1] org.apache.catalina.loader.WebappClassLoaderBase.loadClass   Searching local repositories
26-Nov-2014 19:55:52.294 FINE [localhost-startStop-1] org.apache.catalina.loader.WebappClassLoaderBase.findClass     findClass(org.broadleafcommerce.core.catalog.domain.ProductImpl)
... ProductImpl is not transformed at all

As you can see, the EhCache classes are being loaded directly prior to ProductImpl, and ProductImpl then skips transformation.

Not sure yet if this is the definitive cause, digging in more.

@phillipuniverse
Copy link
Contributor Author

Ok, Ehcache was a false positive. However, it looks like I have found the problem for ProductImpl, I assume that all of the other classes will work in a similar fashion. Here's what I did:

  1. Downloaded the Tomcat 8 source code and imported it into Eclipse using the instructions at http://tomcat.apache.org/tomcat-8.0-doc/building.html#Building_with_Eclipse
  2. Ensured that the PROJECT REFERENCE to Tomcat source was available to the debugging instance. This had to be an actual project reference rather than just point to the source jar so that Eclipse would parse conditional breakpoints which is needed for the next part
  3. Set a break point on line 2505 of Tomcat's WebappClassLoaderBase, the start of findResourceInternal
  4. Change the conditions of the breakpoint to name.contains("ProductImpl"). This should now break for any loading of ProductImpl, to determine how/where the map of resourceEntries is populated

I lost a LOT of time debugging at this point because I was using a remote debugger. Because of that, I was starting up the server and then attaching the debugger in eclipse. I got REALLY lucky here after a couple of hours of trying different things where I got really really fast. Because I got the debugger connected really quickly once, I noticed that breakpoint fired VERY VERY early on in the startup process. It was never triggered before because the debugger was always attached too late.

I then modified the server startup in Eclipse to not use a remote debugger but simply debug the running eclipse application. I then got a reliable triggering of the early breakpoint, and then started going back up through the stack frames:

screen shot 2014-11-26 at 9 46 24 pm

To summarize what Tomcat is doing here, it is essentially going through and scanning all of the jars in the lib directory inside of the web application, and then scans every single class in every jar to see if it is a HandlesTypes. It scans all classes and their super classes.

Well, it turns out in the enterprise jars we have a nested inner class for NullProduct that extends ProductImpl:

public interface CatalogCacheService {

    static class NullProduct extends ProductImpl {}
    static final NullProduct NULL_PRODUCT = new NullProduct();

    static class NullCategory extends CategoryImpl {}
    static final NullCategory NULL_CATEGORY = new NullCategory();
...

Tomcat comes across the NullProduct class and loads it up using a FileInputStream, and then goes to load the superclass ProductImpl by using the class loader. At this point, instrumentation has not kicked (this doesn't happen until the persistence context) in and thus the WebAppClassLoader has no transformers associated to it and thus no class transformation takes place.

We'll see what happens with the rest of the classes here, but that is absolutely the cause for ProductImpl. I am not sure yet why this does not happen with every single domain class and they all don't have this problem; I suspect there is an if clause that rules them out that I am missing.

@phillipuniverse
Copy link
Contributor Author

Here are the classes that fail transformation from the log messages:

[ WARN] 22:33:42 MPUM - The class org.broadleafcommerce.common.sitemap.domain.SiteMapGeneratorConfigurationImpl...
[ WARN] 22:33:51 MPUM - The class org.broadleafcommerce.core.catalog.domain.CategoryImpl
[ WARN] 22:33:53 MPUM - The class org.broadleafcommerce.core.catalog.domain.ProductImpl...
[ WARN] 22:33:56 MPUM - The class org.broadleafcommerce.core.order.domain.OrderItemImpl...
[ WARN] 22:33:59 MPUM - The class org.broadleafcommerce.cms.file.domain.StaticAssetImpl...
[ WARN] 22:34:02 MPUM - The class org.broadleafcommerce.cms.url.domain.URLHandlerImpl...
[ WARN] 22:34:04 MPUM - The class com.broadleafcommerce.enterprise.workflow.domain.change.MapStructureImpl...

and their causes that are being loaded by this process. For all of these cases, it is because Tomcat loads the superclass of all of the 'caused by' classes:

Class that is skipping transformation Caused by initially inspecting class
org.blc...SiteMapGeneratorConfigurationImpl org.blc...CustomUrlSiteMapGeneratorConfigurationImpl
org.blc...CategoryImpl com.blc...CatalogCacheService$NullCategory
org.blc.catalog.domain.ProductImpl com.blc...CatalogCacheService$NullProduct
org.blc...OrderItemImpl org.blc...BundleOrderItemImpl
org.blc...StaticAssetImpl org.blc...ImageStaticAssetImpl
org.blc...URLHandlerImpl org.blc...NullURLHandler
com.blc.enterprise...MapStructureImpl com.blc.enterprise...SimpleValueMapStructureImpl

I am not sure how we should fix this. I think that we need to open a support ticket with Tomcat, this feels like a bug in Tomcat to me. This implementation does not let the class transformers run properly.

@phillipuniverse
Copy link
Contributor Author

Relevant comments from Tomcat's ContextConfig class (starting at line 1067) that references a possible way to exclude scanning every single class by marking your web.xml with metadata-complete=true:

    /**
     * Scan the web.xml files that apply to the web application and merge them
     * using the rules defined in the spec. For the global web.xml files,
     * where there is duplicate configuration, the most specific level wins. ie
     * an application's web.xml takes precedence over the host level or global
     * web.xml file.
     */
    protected void webConfig() {
        /*
         * Anything and everything can override the global and host defaults.
         * This is implemented in two parts
         * - Handle as a web fragment that gets added after everything else so
         *   everything else takes priority
         * - Mark Servlets as overridable so SCI configuration can replace
         *   configuration from the defaults
         */

        /*
         * The rules for annotation scanning are not as clear-cut as one might
         * think. Tomcat implements the following process:
         * - As per SRV.1.6.2, Tomcat will scan for annotations regardless of
         *   which Servlet spec version is declared in web.xml. The EG has
         *   confirmed this is the expected behaviour.
         * - As per http://java.net/jira/browse/SERVLET_SPEC-36, if the main
         *   web.xml is marked as metadata-complete, JARs are still processed
         *   for SCIs.
         * - If metadata-complete=true and an absolute ordering is specified,
         *   JARs excluded from the ordering are also excluded from the SCI
         *   processing.
         * - If an SCI has a @HandlesType annotation then all classes (except
         *   those in JARs excluded from an absolute ordering) need to be
         *   scanned to check if they match.
         */
...
...
...

@phillipuniverse
Copy link
Contributor Author

I opened a Tomcat bug at https://issues.apache.org/bugzilla/show_bug.cgi?id=57274.

@phillipuniverse
Copy link
Contributor Author

As I think about this more, dunno what Tomcat can really do about this, if anything. Spring adds class transformers late in the game (after this ServletContainerInitializer scanning takes place), and we do it even later because we don't add transformers until the persistence unit starts up (after all the Spring beans have been initialized). The problem actually affects every single domain class that represents a superclass. I would imagine that Tomcat added the classloader part to parse the superclass and interfaces for a performance improvement on startup. One possible fix on their part would be to disable this performance improvement so that every single class is scanned by looking it up via a FileInputStream (and thus not do classloading for those super classes) but it is unlikely that this will change.

That said, after reading through http://java.net/jira/browse/SERVLET_SPEC-36 (and viewing the commit in Tomcat that I cannot find at the moment), it looks like you can add an absolute-ordering entry to your web.xml to completely disable the ServletContainerInitializer scanning, and thus exclude the early classloading:

<absolute-ordering />

I have tested this out and I can now successfully deploy on Tomcat 8! Woohoo! This also has the added benefit of improving startup time since Tomcat doesn't have to scan all classes in all jars. Also, since that <absolute-ordering /> is a servlet 3.0 spec thing, it will likely behave the same on all other servlet containers, reducing startup time across the board.

By the way, with Tomcat 8 (and I think later versions of Tomcat 7? Need to go confirm exactly which versions) there is no need at all to specify the spring-instrument jar as a javaagent. Since Tomcat 8 now has native support for class transformations spring-instrument doesn't do anything.

This whole process is somewhat concerning though as it has brought to light some fundamental problems with class transformation. I am not sure what the answer is here other than to be mindful of these potential issues.

I also have a few logging changes that I will push up to distinguish between ERROR states that will cause really bad problems (within the classes above) and misconfigurations, then I think this issue can be closed.

phillipuniverse added a commit that referenced this issue Nov 29, 2014
… transforming and throw an exception if we detect that a class could not be transformed
@phillipuniverse
Copy link
Contributor Author

The changes that I made are to:

  1. Log a warning whenever a class is detected as managed by the MergePersistenceUnitManager but doesn't have an @Entity, @Embeddable or @MappedSuperclass annotation on it. This is a misconfiguration as that class would have to be referenced in some persistence.xml but does not necessarily represent an error state
  2. Log an error and throw an IllegalStateException when a class is an entity class (meaning, has the above annotations) and yet is not registered as having been transformed by the class transformers. Since it is very hard to determine that this happened later on in application, I thought it was best to throw an exception and completely prevent the remainder of starting up to force the user to resolve it. @jefffischer @bpolster do you have a different opinion on what to do in this case?

I am committing this change to both 3.1 and 3.2. Even though we just released 3.1.9-GA, I do not think it is necessary to release a high-priority 3.1.10-GA patch release as this can be quickly resolved within individual users' applications (by adding <absolute-ordering /> into their web.xml). My "fix" is to basically add some helpful debugging and detection code.

Also, I went back through today and verified my changes with JRebel. Both JRebel 6.0.0 and JRebel 6.0.1 do not work with Tomcat 8 and our class transformers. There are actually a TON of entity classes that do not get transformed by our transformers (way more than the original set above). Unfortunately since JRebel is closed source, I cannot go and debug their obfuscated code for class loading 👎. -1 points for closed source proprietary code.

I did use JRebel 6 on Tomcat 7 and had no problems with class transformation there. I have opened an issue with JRebel located at http://zeroturnaround.com/forums/topic/tomcat-8-and-spring-class-transformation/.

I am going to keep this issue open until hearing back from JRebel.

@RapidTransit
Copy link

@phillipuniverse I had some problems with JRebel over the weekend (Gradle, Jrebel, Tomcat and IntelliJ what a mess, I think my site will be done by 2020 at this rate :/) and hit the forums and noticed the issue you were having. Probably not the answer you're looking for but, take a look at http://zeroturnaround.com/software/jrebel/learn/jrebel-plugins/

phillipuniverse added a commit to BroadleafCommerce/LegacyDemoSite that referenced this issue Dec 3, 2014
…etContainerInitializers according to the Servlet 3.0 spec
@phillipuniverse
Copy link
Contributor Author

@RapidTransit thanks for the suggestion. I don't think the plugin architecture doesn't actually make sense in this case. JRebel already triggers a reload of the Hibernate session on changing a domain class. Because the sessions reloads, it reloads the MergePersistenceUnitManager and thus goes back through our class transformation.

Also, one of the Jrebel devs replied to my form post and said they have a fix to using TomcatInstrumentableClassLoader in a nightly release. I will be testing with that and then this issue should be resolved.

@RapidTransit
Copy link

@phillipuniverse just a last resort in case of a can't fix/won't fix

@phillipuniverse
Copy link
Contributor Author

Per the JRebel issue that I linked, as of the JRebel Dec-05, 2014 @ 14:37 UTC 6.0.2-SNAPSHOT this has been fully resolved. For those following this thread, our official Tomcat 8 support with JRebel will only be for 6.0.2 and above.

@phillipuniverse phillipuniverse changed the title Issues with Spring LoadTimeWeaving/Class Transformation on recent Tomcat versions Issues with Spring LoadTimeWeaving/Class Transformation on Tomcat 8 Dec 5, 2014
@phillipuniverse
Copy link
Contributor Author

One final note here. Session persistence in Tomcat should always be disabled. We currently store full instances of the following classes in session:

org.broadleafcommerce.common.currency.domain.BroadleafCurrencyImpl
org.broadleafcommerce.common.locale.domain.LocaleImpl
org.broadleafcommerce.profile.core.domain.CustomerImpl

If you use Tomcat's session persistence, then when Tomcat starts up it will load all of the session data, thus triggering a class load of these classes before the persistence manager.

This can be disabled in context.xml via the <Manager> element (as referenced in http://tomcat.apache.org/tomcat-6.0-doc/config/manager.html#Disable_Session_Persistence):

<Context>
    <!-- disable session persistence -->
    <Manager pathname="" />
</Context>

Re-opening to provide more logging around disabling session persistence.

@phillipuniverse
Copy link
Contributor Author

Sadly this issue must be resurrected, as the fix that I proposed (using <absolute-ordering /> in web.xml) does not work when you don't have a web.xml! Scanning still happens, still communicates with the classloader to load super classes so any time we have an subclass of a domain object it doesn't get transformed because the transformers are not registered. It appears that this only happens starting with Tomcat 7.0.70, and I verified that this problem exists from 7.0.70 up through 7.0.78 (I tested with all versions). This original issue was reported with Tomcat 8, but after more research it appears that this issue has been fixed in Tomcat 8.0.33+ (which I assume also includes 8.5.0+) so presumably we'd see the same problem without a web.xml (and thus without absolute-ordering) in 8.0.0 up to 8.0.32.

For completeness sake, there is also a way to prevent jars from being scanned by this process with the property org.apache.catalina.startup.ContextConfig.jarsToSkip= in conf/catalina.properties. Example: org.apache.catalina.startup.ContextConfig.jarsToSkip=broadleaf-*.jar will skip scanning all Broadleaf jars. However, you still have to exclude any client-side jars (like the core jar) since almost invariably those will be subclasses of Broadleaf domain classes. However, after looking at the code this will still be an issue for classes in /WEB-INF/classes (any classes that are actually in the .war project) and there is no way to exclude that scanning. So if in the site or admin project you have a domain class that extends from a Broadleaf domain class then you will see the issue and there is no way to fix it, unless you move them into the core jar and skip the scanning.

Since not using a web.xml is the default for our 5.2 starter projects we will need to provide the above caveat with Tomcat 7.0.70+ and 8.0.0->8.0.32.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants