generator-jhipster 🚀 - Upgrade to Spring Boot 1.5.14 and 2.0.3

Brussels-SR11 expected tomorrow!

DanielFran on 15 Jun 2018

For Spring Boot 2.0.3 the build is currently failing, it looks like they changed the way Ehcache is configured, I'm on it.

jdubois on 15 Jun 2018

I tried to update a 4.x app to 1.5.14 this morning and had also problems with the cache configuration. Somehow it seems the order has changed again as it fails because ehcache is configured after datasource again. Works with 1.5.13 and applying your fix from this commit: https://github.com/jhipster/generator-jhipster/commit/843a671cf2e9febb20c1749fab7c2d522062ee1e

atomfrede on 15 Jun 2018

Oh and my trainees also had this problem recently - what is happening here, this is making everything fails :-(
Thanks for the link to the commit!

jdubois on 15 Jun 2018

Nope @atomfrede - this is probably something close, but the fix doesn't work for me... In fact I have 2 caches, one created before (the correct one, with our setup), and then another one. It looks like Hibernate doesn't see the existing Ehcache instance and creates a second one.

jdubois on 15 Jun 2018

So for Spring Boot 2.0.3 this fails because we use our own io.github.jhipster.config.jcache.NoDefaultJCacheRegionFactory for the Hibernate hibernate.cache.region.factory_class property.

The idea is that if you don't configure your cache for each entity, it won't be cached. So our class is a hack that makes the app crash if those caches are not configured correctly, which I find is a good idea.

Then that class is kind of a hack, and for some reason it doesn't work anymore. I still find the idea very good, then it's also a bit against our Policy 1 - even if in that case the default config is pretty stupid (making you think your class is cached, when it is not...).

I'll have a closer look, but otherwise let's just remove this.

jdubois on 15 Jun 2018

👍1

This is so buggy...
So removing hibernate.cache.region.factory_class was a bad idea:

Everything is correct in the code (the @Cache annotations, etc)
At start up you have logs telling that the caches are created
But in fact nothing goes into the cache!!!! It's just ignored

That's a really bad default configuration from Spring Boot! So let's not do this, and I'm stuck again :-(

jdubois on 15 Jun 2018

I think this part was coded by @henri-tremblay - Henri, does that ring a bell to you?

jdubois on 15 Jun 2018

A quick summary as I don't know if I'll have more time on this today.

With this new Spring Boot 2.0.3 release:

We do have CacheConfiguration correctly setting up the caches, using JSR 107
But the Hibernate entity manager does not "see" that JSR 107 instance, so it creates a new one, in which the caches are not correctly created (this second instance has no cache configured)
As we have io.github.jhipster.config.jcache.NoDefaultJCacheRegionFactory which is checking for non-configured cache (read http://www.ehcache.org/blog/2017/03/15/spontaneous-cache-creation.html to understand why this is a good idea), this creates an error in this second JSR 107 instance, and this makes the application crash

So:

io.github.jhipster.config.jcache.NoDefaultJCacheRegionFactory is a really great thing, we should definitely keep it
We need to make the Hibernate entity manager "see" the existing instance, like it used to work before. I have no idea how to do that at the moment.

Any help here would be greatly appreciated!

jdubois on 15 Jun 2018

And I think its the same for 1.5.14 at least thats what I can see in the logs (without investigating any further)

atomfrede on 15 Jun 2018

Hi. Just waked up :-) I will have a look today. NoDefaultJCacheRegionFactory is indeed a good thing. It is actually the default in the latest Hibernate version so we will be able to get rid of it. The JSR107 CachingProvider is a singleton. If Hibernate and Spring are not seeing the same cache manager, it means both are not on the same URI anymore.

henri-tremblay on 15 Jun 2018

Thanks @henri-tremblay !! Indeed that's supposed to be a singleton, but caches are initialized twice, here are my logs if I use a org.hibernate.cache.jcache.JCacheRegionFactory instead of the NoDefaultJCacheRegionFactory:

2018-06-15 15:29:06.481 DEBUG 55270 --- [  restartedMain] c.ehcache.core.Ehcache-usersByLogin      : Initialize successful.
2018-06-15 15:29:06.509 DEBUG 55270 --- [  restartedMain] c.ehcache.core.Ehcache-usersByEmail      : Initialize successful.
2018-06-15 15:29:06.513 DEBUG 55270 --- [  restartedMain] c.e.c.E.mycompany.myapp.domain.User      : Initialize successful.
2018-06-15 15:29:06.517 DEBUG 55270 --- [  restartedMain] c.e.c.E.m.myapp.domain.Authority         : Initialize successful.
2018-06-15 15:29:06.521 DEBUG 55270 --- [  restartedMain] c.e.c.E.m.myapp.domain.User.authorities  : Initialize successful.
2018-06-15 15:29:06.525 DEBUG 55270 --- [  restartedMain] c.e.c.E.mycompany.myapp.domain.Foo       : Initialize successful.
2018-06-15 15:29:06.688 DEBUG 55270 --- [  restartedMain] c.m.myapp.config.DatabaseConfiguration   : Configuring Liquibase
2018-06-15 15:29:06.811  WARN 55270 --- [mple-Executor-1] i.g.j.c.liquibase.AsyncSpringLiquibase   : Starting Liquibase asynchronously, your database might not be ready at startup!
2018-06-15 15:29:07.511 DEBUG 55270 --- [mple-Executor-1] i.g.j.c.liquibase.AsyncSpringLiquibase   : Liquibase has updated your database in 698 ms
2018-06-15 15:29:07.627 DEBUG 55270 --- [  restartedMain] c.e.c.E.mycompany.myapp.domain.User      : Initialize successful.
2018-06-15 15:29:07.674 DEBUG 55270 --- [  restartedMain] c.e.c.E.mycompany.myapp.domain.Foo       : Initialize successful.
2018-06-15 15:29:07.678 DEBUG 55270 --- [  restartedMain] c.e.c.E.m.myapp.domain.Authority         : Initialize successful.
2018-06-15 15:29:07.816 DEBUG 55270 --- [  restartedMain] c.e.c.E.m.myapp.domain.User.authorities  : Initialize successful.

-> the first Initialize successful lines come from our CacheConfiguration, then Liquibase kicks in, and then it's Hibernate that is starting its own cache, giving identical Initialize successful lines, for caches that are created on the fly. That's also why you only have the first 2 lines (usersByLogin and usersByEmail) only on the first pass, as those are Spring Cache only caches (not JPA entities).

In that configuration, the second caches are used, so it looks like it's working, but it's not using the caches that we have configured in CacheConfiguration, which is wrong. And hard to spot if you don't look at the logs!

I'm sorry but I won't have much time this week-end on this, and this is of course very important, so help is really, really welcome.

jdubois on 15 Jun 2018

Ok. And where are you testing it? To you have a branch or a test project I can try? Only the Spring-boot version was changed? Not the hibernate one for instance?

henri-tremblay on 15 Jun 2018

I'm using the master branch from "jhipster/jhipster" and do a "mvn clean install" so I have version 2.0.12 in my local Maven repo.
Then I generate a project and upgrade JHipster dependencies in the pom.xml (just one line to point to 2.0.12).
I'm doing this from my phone, so not sure it's 100% correct, but this is the general idea.

jdubois on 15 Jun 2018

So. The problem is there because Spring and Hibernate are no longer using the same class loader to find their CacheManager. Now the question is "why?". Which I will investigate right away.

henri-tremblay on 15 Jun 2018

Ok. Here is the spring-cache code that broke everything: spring-projects/spring-boot/pull/13338. However, the fix seems good so the problem is now to pass the right classloader to hibernate.

@snicoll You never experienced this in one of your samples?

henri-tremblay on 15 Jun 2018

Here are two solutions. They are not pretty but they work. Due to the fact that Hibernate can't take a Spring bean as a region factory, I don't see what else I can do unless something is modified in Hibernate or Spring (not sure what though).

Solution 1: Just guess the class loader by taking the one of the RegionFactory instance. Since Spring is loading Hibernate, in general it should work.

public class ClassLoaderFixJCacheRegionFactory extends NoDefaultJCacheRegionFactory {

    @Override
    protected CacheManager getCacheManager(Properties properties) {
        CachingProvider cachingProvider = getCachingProvider( properties );
        String cacheManagerUri = getProp( properties, CONFIG_URI );
        ClassLoader classLoader = getClass().getClassLoader();

        URI uri = getUri(cachingProvider, cacheManagerUri);
        return cachingProvider.getCacheManager(uri, classLoader);
    }

    private URI getUri(CachingProvider cachingProvider, String cacheManagerUri) {
        URI uri;
        if (cacheManagerUri != null) {
            try {
                uri = new URI(cacheManagerUri);
            }
            catch (URISyntaxException e) {
                throw new CacheException("Couldn't create URI from " + cacheManagerUri, e);
            }
        }
        else {
            uri = cachingProvider.getDefaultURI();
        }
        return uri;
    }
}

Solution 2: Take the real spring class loader and hide it in a static variable.

public class BeanClassLoaderAwareJCacheRegionFactory extends NoDefaultJCacheRegionFactory {

    private static volatile ClassLoader classLoader;

    @Override
    protected CacheManager getCacheManager(Properties properties) {
        Objects.requireNonNull(classLoader, "Please configure this class as a Spring Bean to get a class loader correctly");

        CachingProvider cachingProvider = getCachingProvider( properties );
        String cacheManagerUri = getProp( properties, CONFIG_URI );

        URI uri = getUri(cachingProvider, cacheManagerUri);
        CacheManager cacheManager = cachingProvider.getCacheManager(uri, classLoader);

        // To prevent some class loader memory leak this might cause
        setBeanClassLoader(null);

        return cacheManager;
    }

    private URI getUri(CachingProvider cachingProvider, String cacheManagerUri) {
        URI uri;
        if (cacheManagerUri != null) {
            try {
                uri = new URI(cacheManagerUri);
            }
            catch (URISyntaxException e) {
                throw new CacheException("Couldn't create URI from " + cacheManagerUri, e);
            }
        }
        else {
            uri = cachingProvider.getDefaultURI();
        }
        return uri;
    }

    public static void setBeanClassLoader(ClassLoader classLoader) {
        BeanClassLoaderAwareJCacheRegionFactory.classLoader = classLoader;
    }
}

The second solution is uglier but will always work.

henri-tremblay on 15 Jun 2018

Thank you so much for the analysis! I'll have a look on Sunday (no computer ATM and this needs testing).
I'm worried the static variable could cause issues on an app server with multiple WARs, but who does this nowadays?

jdubois on 16 Jun 2018

Yes. But this is for a little while. So it causes issues only if you are crazy enough to have a factory in a class loader common to two wars that are deployed at the same moment. The alternative is to use a thread local and assume Spring is always loaded in a single thread.

henri-tremblay on 16 Jun 2018

Here is the spring-cache code that broke everything

If you have a sample that shows how it breaks, I am happy to revisit the code.

@snicoll You never experienced this in one of your samples?

We don't have support for 2nd level cache in Spring Boot and, as far as I understand, this is why you're having this issue. I'd be happy to include such support in Spring Boot itself but never had the time to look at it (PR welcome).

IMO, the code that was changed is better than the previous version. I might change my mind looking at the specifics though.

snicoll on 16 Jun 2018

Thanks, @snicoll. Yes. It's not a Spring issue. Using this class loader make sense. The real problem is a mix of JSR 107 playing with class loaders and Hibernate factory that can't be injected from a Spring bean.

@jdubois A stupid solution could be to remove second level caching from Hibernate by arguing that the Spring cache layer should be enough.

henri-tremblay on 16 Jun 2018

I'm quite happy that JHipster provides Hibernate 2nd level cache on top of Spring Boot :-)

For the record, for the last month, for all JHipster generated projects:

80% of all projects used an SQL database
70% of all projects used Hibernate 2nd level cache (which means 87,5% of projects using an SQL database used an Hibernate 2nd level cache)
80% of all projects used Spring Cache (usually with Hibernate 2nd level cache, but not always - for example for MongoDB users)
52% of all projects used Ehcache!

jdubois on 17 Jun 2018

Concerning the solutions: @henri-tremblay I guess solution 2 is better, as you said it will always work. It will be hidden in our library, so it's not too bad if the code isn't perfect.

We'll probably also need this for Spring Boot 1.4, so I'll backport it there.

I'm testing this!

jdubois on 17 Jun 2018

@henri-tremblay solution 1 in fact doesn't work with my setup - I don't understand why...
For solution 2, my problem is that it needs to be configured as a Spring Bean: if we auto configure it in the jhipster/jhipster lib, we would then need to add a property to disable it if needed. So in the end this is quite a complex configuration.
So at the moment I'd rather have solution 1, but I need to make it work on my machine.

-> if you have the time, could you do a PR? That way you would still be the author of the code, and that would prove that it works (through Travis)

jdubois on 17 Jun 2018

I much prefer solution 2. And I think it is easier to convert into a PR to Spring. Right now I was injecting it as a bean in CacheConfiguration. But yes, I will do a PR.

henri-tremblay on 17 Jun 2018

Yes you are right @henri-tremblay !

About solution 1, I found my issue: I was putting the class into the jhipster/jhipster lib, so it wasn't in the right classloader. I had to move my class into the project, so it has to be generated by jhipster/generator-jhipster, and that's indeed awful.

So I'd much prefer solution 2, if it can be put inside jhipster/jhipster, but that looks a bit hard to do. I'll also try to do it.

jdubois on 17 Jun 2018

@henri-tremblay got it... It's not even a Spring Bean... Really ugly code, but very easy to use from the user's point of view, and no risk to fail. I'll commit this and let you tell me what you think of it.

jdubois on 17 Jun 2018

For the record, a modified version of solution 2 was commited in jhipster/jhipster at https://github.com/jhipster/jhipster/commit/662096ae3ba861b82f3e01f3566322ec93be8a20

jdubois on 17 Jun 2018

Do you need a PR in generator-jhipster? Or you will take care of it. The code I used for far was

@Configuration
@EnableCaching
public class CacheConfiguration implements BeanClassLoaderAware {

    // [...]

    @Override
    public void setBeanClassLoader(ClassLoader classLoader) {
        BeanClassLoaderAwareJCacheRegionFactory.setBeanClassLoader(classLoader);
    }
}

henri-tremblay on 18 Jun 2018

Thanks @henri-tremblay I did it in https://github.com/jhipster/generator-jhipster/commit/51bbc386e75db690bec2b73cb6c792b9e3453b62 but I was a bit more "brutal" than you - is that OK for you?

And thanks so much for your hard work here, I wouldn't have found this alone!!!

jdubois on 18 Jun 2018

That's fine. It will work :-)

henri-tremblay on 18 Jun 2018

👍1

Brussels-SR11 has been released on http://repo1.maven.org/maven2/io/spring/platform/platform-bom/Brussels-SR11/ -> time to upgrade JHipster 4!

jdubois on 18 Jun 2018

I upgraded (and tested!) everything for a new JHipster v4 release, using Spring Boot 1.5.14 - the builds are going to be falky in the next few hours as Maven central won't be updated, I'll restart them later and do the release tomorrow morning!

jdubois on 18 Jun 2018

👍1

I think a cleaner solution would be a programmatic configuration of the EntityManager in order to provide a ServiceContributor from Spring. But I don't know how to do it right now.

henri-tremblay on 18 Jun 2018

So the JHipster 4 builds are failing, but this is because of the React tests - I guess this is a misconfiguration as we don't support React with JHipster 4!!! So this isn't a blocker, and I'm doing the release.

jdubois on 19 Jun 2018

@jdubois but for the 4.x branch there shouldn't be any React test config in Travis right as its a different branch?

deepu105 on 19 Jun 2018

can this ticket be closed?

deepu105 on 19 Jun 2018

@deepu105 not sure what happens with Travis, but I don't think this is very important as the v4 branch is in maintenance mode now.

And I'll close the ticket as soon as I finish a couple of tests.

jdubois on 19 Jun 2018

For the record, I think the final solution will be to configure Hibernate programmatically. I will look into it but if someone is an expert, I'm all ears.

henri-tremblay on 20 Jun 2018

For the record, I think the best solution would be to configure Hibernate programmatically and inject a RegionFactory that is Spring aware. I'll have a look but if some Hibernate+Spring wants to, I'm all ears.

henri-tremblay on 21 Jun 2018

@henri-tremblay I am working in a PR to migrate to spring-boot 2.1.0.x (M1 for now, but probably M2 next week).
I see now that spring migrated actually to hibernate 5.3.3.Final and as you know well the JCacheFactory has been changed (https://hibernate.atlassian.net/browse/HHH-12702?jql=project%20%3D%20HHH%20AND%20fixVersion%20in%20(5.2.18%2C%205.3.0.Beta1%2C%205.3.0.Beta2%2C%205.3.0.CR1%2C%205.3.0.CR2%2C%205.3.1%2C%205.3.2%2C%205.3.3%2C%205.3.4%2C%205.3.5)%20AND%20component%20%3D%20hibernate-jcache%20ORDER%20BY%20updated).

Since the current implementation in jhipster project is not complaint anymore, I would like to confirm with you what should be the new solution to avoid spontaneous cache creation?

See: https://github.com/DanielFran/jhipster/tree/spring-boot-2.1.0

Thanks

DanielFran on 17 Aug 2018

@DanielFran I will have a look this week.

henri-tremblay on 20 Aug 2018

👍1

So. We can remove the NoDefaultJCacheRegionFactory and replace it with a configuration hibernate.javax.cache.missing_cache_strategy=fail to keep the current configuration. Hibernate uses create-warn as a default right now. To have a "still works but warns you"behaviour to be nice to their users. That could work as well depending on how hard we want to be on it.

It means that BeanClassLoaderAwareJCacheRegionFactory can now inherit JCacheRegionFactory. And be replace by the following code:

public class BeanClassLoaderAwareJCacheRegionFactory extends JCacheRegionFactory {

    private static volatile ClassLoader classLoader;

    /**
     * This method must be called from a Spring Bean to get the classloader.
     * For example: BeanClassLoaderAwareJCacheRegionFactory.setBeanClassLoader(this.getClass().getClassLoader());
     *
     * @param classLoader The Spring classloader
     */
    public static void setBeanClassLoader(ClassLoader classLoader) {
        BeanClassLoaderAwareJCacheRegionFactory.classLoader = classLoader;
    }

    @Override
    protected ClassLoader getClassLoader(CachingProvider cachingProvider) {
        return Objects.requireNonNull(classLoader, 
            "Please set Spring's classloader in the setBeanClassLoader method before using this class in Hibernate");
    }
}

I can PR on master if it is now appropriate.

Also, that said, it should actually be possible to get rid of BeanClassLoaderAwareJCacheRegionFactory but I failed so far. Hibernate allows to plug things into their SPI so a SpringAwareJCacheRegionFactory should be possible. This class would be a Spring bean knowing about the right class loader to use. If someone move versed into Hibernate SPI wants to have a look, feel free.

henri-tremblay on 21 Aug 2018

👍1

Hi @henri-tremblay.

Thanks for your feedback.

I commited my changes in https://github.com/jhipster/jhipster/pull/88 and included your suggestion.

Do you want I remove it and you PR yourself those changes?

DanielFran on 22 Aug 2018

No that's all right. I will survive not being the committer :-)

I think EXCEPTION_MESSAGE could be package scope instead of public.

I wasn't aware of the LogbackRecorder which is pretty handy. Cool!

henri-tremblay on 22 Aug 2018

Thanks for all your comments, I made the changes.

DanielFran on 22 Aug 2018

@jdubois @henri-tremblay Maybe the issue with the Cache 'order' will be fixed in next release: see https://github.com/spring-projects/spring-boot/issues/14181

DanielFran on 24 Aug 2018

@henri-tremblay @DanielFran I also add a try this morning on our spring-boot_2.1.0 branches, and indeed the L2 cache isn't configured properly.

We have warnings like Missing cache[com.mycompany.myapp.domain.User.authorities] was created on-the-fly. The created cache will use a provider-specific default configuration: make sure you defined one. You can disable this warning by setting 'hibernate.javax.cache.missing_cache_strategy' to 'create'.
The caches are correctly configured (I can see them in JMX), but are not used by Hibernate, it seems like it doesn't "see" the configured caches, and hence creates new ones

I don't know what the issue is here, but @henri-tremblay you seem to understand this very well, so help is really welcome. And if you want contributor access that can also be easily solved, if you want to be a "core dev team" member.

jdubois on 21 Nov 2018

👍1

For reference, this is the changes applied: https://github.com/jhipster/jhipster/commit/bf254179dee5f89ed431d348542b8c80397f913f

DanielFran on 21 Nov 2018

Ok. I'll try to have a look today. This comes from changes discussed with Hibernate team. I need to put it back in my head and it shouldn't be too hard to fix.