Cache with read-through cache loader throws NPE #119
Comments
|
ahh, sorry about that. I relied on the TCK with additional tests based on code coverage and discovered bugs. Unfortunately the TCK turns out to be pretty poor and admittedly I put most of my effort towards Caffeine's core test suite. The adapter is a lot larger than I'd like due to significant differences in designs. Since there might be other mistakes, can you use a SNAPSHOT? I figure you'll be banging on it a bit over the next few days and we can avoid version noise. If that's okay, then I'd bump the release when you gave the thumbs up. |
|
Sure, sounds good. It might take a little longer than a few days, "real-work" is getting in the way lately. But I'll be playing with it when I have spare time. |
|
Thanks! Just let me know when you're moving off either due to lack of time or no more issues found. I think we can delay because JCache has had low adoption, isn't the primary API, and the module has a tiny download rate (253 in July according to Central). |
|
It seems that |
|
Snapshot should be good now that the CI behaves correctly again. |
This is required by the specification (per JavaDoc) but the TCK does not validate it, so sadly I failed to handle that case. To get CI functioning again, moving back to a jdk8-only config. I don't know why the new config hangs with Guava's tests and think it might be the distro. Also fixed the misnamed method for DI factories. I'm going to argue it doesn't break semvar since JCache is the API and this is an impl hook no one calls directly.
This is required by the specification (per JavaDoc) but the TCK does not validate it, so sadly I failed to handle that case. To get CI functioning again, moving back to a jdk8-only config. I don't know why the new config hangs with Guava's tests and think it might be the distro. Also fixed the misnamed method for DI factories. I'm going to argue it doesn't break semver since JCache is the API and this is an impl hook no one calls directly.
|
I had to take a break for a while there, but I picked this back up a few weeks ago. I've been using the cache loader with read-through from the snapshot in production for about a week now and haven't found any more issues. |
|
Thanks! Let me sort out #127 and then I'll get you a proper version. |
|
Does it look like 2.3.4 will be out soon. It's looking like #127 is fixed? |
|
I'd like to wait until JCTools is released with the fix, in case additional tests are going to be added. But if not then done soon then I'll go ahead regardless and apply the changes, which are non-invasive. Its critical enough that I won't delay it beyond this week and will have a release out over the weekend regardless of JCTools' status. |
|
Released 2.3.4 with this fix. Thanks for being patient. |
Whenever a
CacheLoader.load()returns a null, theJCacheLoaderAdapter.load()doesn't check that the value is not null before wrapping it in anExpirablewhich doesn't allow nulls.Adding
return value == null ? null : new Expirable<>(value, expireTimeMS());on line 81 seems to fix the failing test.https://github.com/ben-manes/caffeine/compare/master...chrisstockton:read-through-load-null?expand=1
CacheLoader.loadAll()also has the same problem and is similarly fixable, but hopefully it's not as common to return a null instead of any empty map.The text was updated successfully, but these errors were encountered: