Generator-jhipster: Use simple cache instead of Guava's?

[erikkemperman]

I am packaging my app without springfox, in the production profile, and would now like to get rid of the large guava dependency.

However, it's being used by PersistentTokenRememberMeServices.java (for session authentication) and OAuth2AuthenticationService.java (for gateway/uaa authentication).

But it seems to me that in those places, it is actually a bit heavy-handed to use guava's Cache class -- which is specifically designed to be safe for use by multiple concurrent threads, but here we are bluntly synchronizing on a global lock around it!

https://github.com/jhipster/generator-jhipster/blob/master/generators/server/templates/src/main/java/package/security/_PersistentTokenRememberMeServices.java#L118
https://github.com/jhipster/generator-jhipster/blob/master/generators/server/templates/src/main/java/package/security/oauth2/_OAuth2AuthenticationService.java#L151

Would you consider a PR which replaces this use of guava's cache by a simple custom implementation? It's basically just a map with a maintenance thread. I have it mostly done already, and it works like a charm.

But I figured I'd check first, before I spend too much time polishing it and adding unit tests :-)

• [x] Checking this box is mandatory (this is just to show you read everything)

[jdubois]

• yes you are totally correct, and that's a very good idea
• can we use Spring Cache's abstraction, to use some simple HashMap? I haven't looked too much, but I guess it's doable - one of the reasons is that I would like to be able to use a Memcache implementation in the future, so those caches could easily be distributed

[erikkemperman]

Would it be all right to do it in two steps? First a simple map (have that working already) and then go to spring cache?

[gmarziou]

Spring Cache abstraction uses a ConcurrentHashMap by default

[jdubois]

So yes I would rather use Spring Cache

[erikkemperman]

Well, the concurrent map doesn't have any benefits if we call it from within a globally locked section but sure, I'll see if I can make it work with spring cache!

[ramzimaalej]

I have a question about the globally locked section. How would it solve the problem of receiving two requests when we have 2 instances of the app running. Do we really need the global lock?

[atomfrede]

What about using caffeine isntead of guava cache? It supports jcache and spring cache supports caffeine out of box.

[jdubois]

Yes Caffeine looks like the new cool thing, we need at least to study it

[atomfrede]

We have used it already as a replacement for guava caches and it works quite well. But we are not using jcache yet.

[erikkemperman]

FYI I have submitted a PR #6666 which gets rid of Guava but doesn't yet, due to lack of time, use spring caches instead.

[ben-manes]

Please let me know if I can be of any help wrt Caffeine.

[jdubois]

• I really don't think that's a good idea to generate our own cache -> at least it should be in the JHipster server lib, but still I'd rather have it using Spring Cache
• Speaking of Spring Cache, we have this new ticket #6796 to improve it, and that would be the right place to add Caffeine support (ping @ben-manes ), and then use it here
• I don't understand why you add a TLD library?
• Indeed there's a lot of code that must be moved to jhipster-dependencies - I hope that won't be a common issue now that we split our code in different projects!

[erikkemperman]

@jdubois Agreed it would be better to use Spring Cache, but I haven't looked into that further, yet. My main purpose here was to make it possible to exclude Guava from the build-path, which involved replacing the use of Guava cache for the tokens and also its TLD feature.

Obviously anyone with expertise here could try and replace my little custom cache here (actually just a map with a maintenance thread) is very welcome to add to this PR!

I have some ideas about the problem of fragmented code base, just a preliminary suggestion at this point but if you're interested please see my post in the dev mailing list just a little while ago.

[erikkemperman]

Sorry, maybe should have mentioned, the PR I refer to is here:
https://github.com/jhipster/generator-jhipster/pull/6666

[ben-manes]

If you want to drop required dependencies, then @erikkemperman's implementation seems reasonable. It uses a similar write-order queue for fixed expiration, letting it be O(1) rather than a periodic full scan. That's what Guava and Caffeine do, too.

My only complaint is that I dislike spinning up dedicated threads in library code, e.g. for every tiny cache. That's a heavy-weight and error prone, such as will the cache every go out of scope and leave a dangling worker thread? Can you assume JDK9 and use the new common scheduler thread available in CompletableFuture? Will there be multiple instances each needing their own thread?

If jhipster is used on power conscious devices then the thread wake ups will stop it from using a low power state. You might instead schedule the wake-up directly based on the expiration time using a ScheduledExecutorService instead of a raw thread. If there is a shared local scheduler in jhipster, e.g. pre-dating JDK9's, then you could use that to avoid the extra thread.

Both Guava and Caffeine instead trigger the maintenance on cache a read/write rather than using a thread. Since the expiration is O(1), the cost is a cheap peek at the head of the queue. IdentityHashMap does the same for its referenceQueue to evict weak reference entries. The argument is that the cache already used the memory, its likely not going to be a large cache, and inactivity likely means there isn't memory pressure anyway. This would be my preference for that PR.

I haven't used Spring since 1.x - 2.0 days, so I can't comment on that much. But I do know the default cache is an unbounded map without any intelligent features, so I suspect you can't use that here. You might make @erikkemperman's a Spring cache for default internal reuse, but then you may decide adding a dependency like Caffeine is cleaner if users will have to override due to synchronization overhead.

[erikkemperman]

@ben-manes Wow, thanks for the detailed feedback! I should have some time tomorrow to process it properly.

[erikkemperman]

@ben-manes I take your point about thread overhead, although in this case the cache lives in a persistent singleton instance and thus there's only ever just the single thread. I don't expect JH to typically run on highly power-conscious devices, but that's no excuse to be wasteful of course...

JDK8 is our baseline, at least for now, so ScheduledExecutorService is probably not an option. Spring has a @Scheduled annotation that might work here, but I'll look into just doing the expiration on cache access rather than periodically from a dedicated thread; that makes a lot of sense.

About Spring Cache, so you're saying its unbounded map would likely be a poor choice, even to replace my minimal attempt here? That's... Surprising. But I guess if it wasn't built specifically with (only) fixed expiration in mind, then its purge logic is probably more complex than we'd like in this scenario.

Thanks again!

[jdubois]

FYI I just merged #6796 so it should be easy to add Caffeine support using the Spring Cache abstraction (I wouldn't use it as an Hibernate L2 cache, unless we are sure it works well)

-> @erikkemperman is that good for you, would you be OK to work on a PR? The refactoring was pretty heavy, but I know it quite well know, so I can definitely help to review/debug

[jdubois]

I"m closing this as it has been stuck for nearly 3 weeks -> @erikkemperman if you still want to do the PR (or if anyone else is interested), feel free to do it, it's definitely a good change for the project, and I would happily merge this

[erikkemperman]

@jdubois Ah, yes, forgot about this one -- will take another look as soon as I can!

[jdubois]

Awesome!