Support instantiating CacheLoader and CacheWriter with a Guice injector

[chrisstockton]

Currently any read-through.loader and write-through.writer instances are created using FactoryBuilder.factoryOf("className") which just delegates to Class.forName("className").newInstance(). This is fine for simple loaders, but I find that I want to inject a dependency into my CacheLoader/Writers so that they can connect to some remote persistent storage.

It would be nice if the creation of these loaders could be handled by Guice, if so configured. Here's a quick stab at how it could be done: https://github.com/ben-manes/caffeine/compare/master...chrisstockton:guice-factory?expand=1

It relies on static injection of an injector which doesn't seem super ideal, but considering the alternative is just using Class.forName(...).newInstance(), it at least doesn't seem like a step back.

It would also require someone to make a call to requestStaticInjection(GuiceFactory.class) in a module somewhere, but that seems not too onerous.

[ben-manes]

I do think a DI factory would be nice. I'm also not sure the best way to specify the injector instance either. I dislike static injection due to tests, but the JSR doesn't provide many good options for us to consider. The alternative that the spec authors might have intended is a implementation-specific setting on the CacheManager. The reason being their odd method unwrap(class) to downcast with warnings suppressed. Or maybe we could hack Properties since for legacy reasons it allows Object values, though that seems error prone.

I think we wouldn't want to have any Guice specifics in the library. Maybe instead allow users to plug in their own FactoryBuilder for us to call to get a factory for a class. Then you could provide the GuiceFactory and by default it would delegate to the spec's convenience class. This way others could write bridges to their DI framework. I'd be curious to know if Spring's JCache and CDI's implementations are able to inject into the factory beans.

I'm also curious why you've decided to adopt JCache?

[chrisstockton]

We haven't officially adopted JCache, but we do have several APIs that use Hazelcast and several that use Guava caches. We're also currently looking at Apache Ignite. So using the standard APIs seems like a good idea with Caffeine replacing any existing Guava caches.

I originally was prototyping just using the cache annotations, but (like you know) they turned out too limited for what I wanted to do.

Instead of the annotations, I was looking at just injecting javax.cache.Cache caches everywhere. It'd be easy enough to just have a CaffeineCacheModule and an IgniteCacheModule, etc to make swapping the actual implementations in and out very easy with Guice.

Hazelcast and Ignite both use spring xml configuration so you can do DI pretty easily: https://github.com/apache/ignite/blob/master/modules/core/src/main/java/org/apache/ignite/Ignition.java#L487, though when using Guice, you're binding objects created by Spring, which is weird.

Using the FactoryBuilder approach, could you just add a FactoryBuilder instance to CaffeineConfiguration that could be configured via application.conf, defaulting to one that uses javax.cache.configuration.FactoryBuilder?

That seems like it would keep Guice out of the library, although it wouldn't solve the static injection problem, but at least Caffeine's conscience would be clear :)

[ben-manes]

Its unfortunate that I made TypesafeConfigurator use static methods instead of being an instance. It would have been nice to pass the FactoryBuilder through so that you could define it on the CacheManager and avoid statics. But I supposed you'd have to try to isolate tests by using a generated uri in CachingProvider#getCacheManager. Though that's not trustable because some providers use the URI as a path to the config file and all can return the same instance regardless. I guess the specification is not friendly for running parallel tests within a JVM.

I don't think that I need to add a FactoryBuilder to the CaffeineConfiguration, since it takes in Factory<X> directly on the setters. So it seems to be a responsibility of a "Configurator", in this case the TypesafeConfigurator. So I'd read the setting there, defaulted to a wrapper of FactoryBuilder, and off you go. Let me know if you were thinking something different.

If I was in your shoes, I'd probably write my own annotations or even redefine the CacheResult interceptor. Then you could perform the load atomically using an EntryProcessor wrapping the method invocation, optionally supporting bulk in a custom annotation. There wouldn't be a need for a CacheLoader and the method instance has access to the injected fields. So much simpler, but that doesn't solve the cases of wanting to inject into a CacheWriter, CacheEntryListener, or Copier. For that you'd have to supply the CaffeineConfiguration directly, e.g. by forking TypesafeConfigurator, so the above fix is still useful.

[ben-manes]

A slight variant of defining the FactoryBuilder in application.conf is to have a static method in TypesafeConfigurator where you could set instance. That would still leave a static, but not require static injection as you can use requestInjection to get the Injector during binding, or set it later after the injector is created (e.g. if you manage the startup's lifecycle using Guava's ServiceManager). That would result in cleaner code, imho, and avoid needing to perform a Class.newInstance(...) of the class path in the configuration file. Thoughts?

[chrisstockton]

I think this is what you were getting at with the second approach: https://github.com/ben-manes/caffeine/compare/master...chrisstockton:static-factories?expand=1

javax.cache.configuration.FactoryBuilder is a final class and doesn't implement any interfaces, so I ended up adding some indirection in

public final class TypesafeConfigurator<K, V> {
  private static CacheLoaderFactoryBuilder cacheLoaderFactoryBuilder = FactoryBuilder::factoryOf;
  private static CacheWriterFactoryBuilder cacheWriterFactoryBuilder = FactoryBuilder::factoryOf;

  public static interface CacheLoaderFactoryBuilder<K, V> {
    Factory<? extends CacheLoader<K, V>> getFactory(String className);
  }
  ..
  public static interface CacheWriterFactoryBuilder<K, V> {
    Factory<? extends CacheWriter<? super K, ? super V>> getFactory(String className);
  }
}

Maybe we don't need more than one builder, but I got a little tripped up with the generics on the static methods.

Like you said, this got rid of any problems with static injection in client code and made Class.newInstance(...) unnecessary:

  @Test
  public void testGuiceCacheLoaderFactoryBuilder() {
    System.setProperty("config.resource", "/read-through-test.conf");
    Module module = Modules.override(new CacheAnnotationsModule()).with(new CaffeineJCacheModule());
    Injector injector = Guice.createInjector(module, new AbstractModule() {
      @Override
      protected void configure() {
        Map<Integer, Person> persons =
            Arrays.asList(new Person(1, "Dick"), new Person(2, "Jane"), new Person(3, "Spot"))
                .stream().collect(toMap(Person::getId, Function.identity()));
        bind(PersonCacheLoader.class);
        bind(PersonDao.class).toInstance(new PersonDao(persons));
      }
    });

    GuiceCacheLoaderFactoryBuilder builder = new GuiceCacheLoaderFactoryBuilder<>(injector);
    TypesafeConfigurator.setCacheLoaderFactoryBuilder(builder);

    CacheManager manager = injector.getInstance(CacheManager.class);
    Cache<Integer, Person> cache = manager.getCache("read-through");

    Person p = cache.get(1);
    assertThat(p, is(notNullValue()));
    assertThat(p.getName(), is("Dick"));

    Map<Integer, Person> persons = cache.getAll(Sets.newHashSet(2, 3));
    assertThat(persons.size(), is(2));
    assertThat(persons.get(2).getName(), is("Jane"));
    assertThat(persons.get(3).getName(), is("Spot"));
  }

It does feel a little weird with static setters on TypesafeConfigurator, but it definitely works and keeps Caffeine agnostic.

I guess it comes down to static setters vs Class.newInstance and static injection. I think I agree with you that the static setters are the less messy way to do it.

[ben-manes]

Yeah, I was thinking only one builder would be necessary. I stubbed the code as,

@FunctionalInterface
public interface FactoryCreator {
  <T> Factory<T> factoryOf(String className);
}

public final class TypesafeConfigurator {
  static FactoryCreator factoryCreator = FactoryBuilder::factoryOf;

  public static void setFactoryBuilder(FactoryCreator factoryCreator) {
    factoryCreator = requireNonNull(factoryCreator);
  }
...
  String writerClass = config.getString("write-through.writer");
  configuration.setCacheWriterFactory(factoryCreator.factoryOf(writerClass));
...
}

You should also be able to set the static field in CaffeineJCacheModule. That could be done in the binding stage using requestInjection of a GuiceFactoryCreator instance, as an eager singleton to a provider that does the initialization, or registering a startup stage if you have a start/stop lifecycle. Or if we include javax.inject as a dependency, we could annotate the setter with @Inject so that you could use requestStaticInjection(TypesafeConfigurator.class) if you have a bind(FactoryCreator.class).to(GuiceFactoryCreator.class). The latter seems nicest, actually.

I think we both feel the static setter is ugly, but the lesser evil of the options. Since JCache itself uses statics for the global registry of providers (and therefore managers, etc are globally scoped), putting it as an instance level doesn't buy us anything. So its weird but I don't have any bright ideas to improve it.

[chrisstockton]

Much better! I think I would also prefer the @Inject annotation on the FactoryCreator, but either way works.

Any chance you'd add this to the library? It's definitely good enough for me to continue with what I wanted to do when I started.

[ben-manes]

Yep. If I have time I'll try tonight, but otherwise get a release out over the weekend. Either way you'll have it come Monday morning.

[chrisstockton]

Fantastic! Thanks for all the help.

[ben-manes]

One concern with javax.inject.Inject is that I don't know how Spring's classpath scanning interacts with it. I haven't used it since 2.0 days, but I think that one usually restricted it to your company's package to reduce startup time. I wouldn't want the annotation to cause a breakage due to the standard not supporting optional / providedBy.

Since you've used Spring much more recently than I have, do you know if that's going to cause mayhem?

[chrisstockton]

I don't have any experience mixing Spring annotations with the JSR-330 annotations.

Browsing through the latest spring docs, it looks like you can specify multiple base-packages, including packages in 3rd-party jars. But I don't really know.

[ben-manes]

See JCacheGuiceTest for a full example of what we've been discussing. Its not too different from your code, except that you can encapsulate all of the details within your module. If you have a GuiceFactoryCreator that is @Inject with the Injector, your module should add the bindings

requestStaticInjection(TypesafeConfigurator.class);
bind(FactoryCreator.class).to(GuiceFactoryCreator.class);

I'll release shortly. Just waiting to see the CI finish.

[ben-manes]

dang, I accidentally called that method setFactoryBuilder instead of setFactoryCreator. Oh well, too late now.