Lazy init of access objects for run-once computations

[Leigh Klotz, Jr.]

I'd like to get some guidance on correct, clear, and concise initialization of singleton, read-only access objects

wrapping cached run-once computations, with either lazy or eager initialization.

I brought this up peripherally in

  http://groups.google.com/forum/?fromgroups#!topic/google-guice/u0V97-FZBTQ

but I wanted to avoid taking my attempted threadjacking any further and am starting a new topic.

Here's a sample use case:

A Database provides a table of String keys and values.  I'd like to provide a read-only get(String)->String access

object, and I'd like to initialize it safely, so that the resulting object is thread-safe.  I'd like to express this

concisely, in a way that is clear to code readers so they will be incented to copy the pattern.

Lazy init can be done in Java using the static hack, as descrbied by Bob here

  http://blog.crazybob.org/2007/01/lazy-loading-singletons.html

  and in Wikipedia here

  http://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom 

but I'd like to do it using Guice, for all the usual reasons, such as so I can integrate these providers in with normal injection.

I hope my problem statement is clear. I'm looking for guidance on the best solution.

Here are three unsatisfactory ideas and one maybe OK one. 

Proposal A:

Proposal A does the work of reading the database in the constructor.

This code is clear, and it's certainly concise, and as near as I can understand, it's thread-safe.

Unfortunately, I belive it isn't correct in Guice because of the problems associated with doing work in constructors

(proxy objects, for example).

    @LazySingleton

    class CachedThings {

      private final Map<String,String> cache;

      @Inject CachedThings(DB db) {

        this.cache = readCache(db);

      }

      public String get(String x) {

        return cache.get(x);

      }

      private Map<String,String> readCache(DB db) {

        Map<String,String> result = new HashMap<String,String>();

        for (Row row : db) { map.add(row.a, row.b); }

        return result;

      }

    }

Proposal B:

Proposal B splits the data access object from the database reader operation, moving the database read into its own

Provider, where it can operate safely.  This appears to be just as thread-safe as the previous version, but is

considerably less concise.  It also exposes a @Named TypeLiteral that is not only ugly, but also opens the

possibility of someone directly injecting that Map and causing unwanted expensive database reads.

    @LazySingleton

    class CachedThings {

      private final Map<String,String> cache;

      @Inject CachedThings(@Named("hack") Map<String,String> cache) {

        this.cache = cache;

      }

      public String get(String x) {

        return cache.get(x);

      }

      static class MyModule extends AbstractModule {

         protected void configure() {

           bind(new TypeLiteral<Map<String,String>>(){/**/}.annotatedWith(Names.named("hack")).toProvider(ThingProvider.class).in(LazySingleton.class);        

         }

      }

      @LazySingleton 

      static class ThingProvider implements Provider<Map<String,String>> {

        private final DB db;

        @Inject ThingProvider(DB db) {

          this.db = db;

        }

        private Map<String,String> get() {

          Map<String,String> result = new HashMap<String,String>();

          for (Row row : db) { map.add(row.a, row.b); }

          return result;

        }

    }

Proposal C:

Proposal C uses @Provides methods so avoid to the problematic verbosity of TypeLiteral, and indeed the code is cleaner, but we still have the @Named hack and

internal data exposure.  (I've written the MyModule as a static class of CachedThings, which is questionable, so we might need to deduct a few points

for the additional verbosity needed to move the module out.)

    @LazySingleton

    class CachedThings {

      private final Map<String,String> cache;

      @Inject CachedThings(@Named("hack") Provider<Map<String,String>> thingProvider) {

        this.cache = thingProvider.get();

      }

      public String get(String x) {

        return cache.get(x);

      }

      static class MyModule extends AbstractModule {

         protected void configure() { }

         @LazySingleton @Provides @Named("hack") Provider<Map<String,String>> getThing(DB db) {

           Map<String,String> result = new HashMap<String,String>();

           for (Row row : db) { map.add(row.a, row.b); }

           return result;

        }

      }

    }

Proposal D:

I don't know much about injected methods other than that they run after constructors and the results can't be final.

Is this correct with regard to Guice initialization sequence?  Is it safe for multi-threaded readonly access of the resulting HashMap?

Maybe this is the right solution is to use an @Inject setCache(DB) method?

    @LazySingleton

    class CachedThings {

      private Map<String,String> cache;

      @Inject CachedThings(...) {

        ... other stuff here if necessary ...

      }

      @Inject void setCache(DB db) {

         Map<String,String> result = new HashMap<String,String>();

         for (Row row : db) { map.add(row.a, row.b); }

         cache = result;

      }

      public String get(String x) {

        return cache.get(x);

      }

    }

Thank you,

Leigh.

[Tim Peierls]

I didn't know about a Guice prohibition on doing things in constructors, unless it was to avoid problems with circular dependencies. (So don't have circular dependencies!)

I don't see a problem with Proposals A or D. I have yet to see it documented this way, but the Guice folks have assured me over the years that there is a happens-before edge between writes in @Inject-ed methods and reads of an injected object (unless you try very hard and do evil things during provision). Proposal A looks fine to me; Proposal D looks safe as long as the non-final Map is always accessed read-only after construction/injection (which seems to be the case).

Proposals B and C seem unnecessarily verbose; the use of @Named to inject a Map<String, String> is overkill. If you want to use a @Provides method, isn't this sufficient? 

    @LazySingleton @Provides CachedThings cachedThings(DB db) {

        Map<String, String> cache = readCache(db);

        return new CachedThings(cache);

    }

This last seems like the nicest approach because you get to keep the cache field final, and you keep the reading separate from the access. "readCache" could be a static method of some other class in the same package, so that the Module's only role would be to say "To make the CachedThings from the DB, first read the cache from the DB into a map and construct the CachedThings from that map."

--tim

--
You received this message because you are subscribed to the Google Groups "google-guice" group.
To view this discussion on the web visit https://groups.google.com/d/msg/google-guice/-/HUKKPENRVqcJ.
To post to this group, send email to google...@googlegroups.com.
To unsubscribe from this group, send email to google-guice...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/google-guice?hl=en.

[Fred Faber]

The notion of avoiding heavyweight work in constructors is largely borne out separating the creation of an object graph from executing nontrivial logic in an app.  The object graph creation is essentially a side-effect of injector creation, and we don't have a good reach into this activity.  This prevents us from inclulding some elementary behavior such as error handling.  More optimistically in the no-error case, we want an app to have control as soon as possible instead of blocking on injector creation, and hence want to keep constructors simple and quick.

Lazy initialization is part of the wider topic of object lifecycle.  There's nothing intrinsic within Guice that directly addresses this (minus to some extent type listeners).   However the Service interface within Guava is a convenient solution.  Specifically:  1) define initialization logic as a Service (most often extending AbstractIdleService); 2) bind your services within a multibinder (Set<Service>); and, 3) after your injector is created, iterate over this set and start each service.

For (3), creating a wrapper to do the iteration is convenient so that you can more easily parallelize the initialization.  There's a candidate for this to be released within Guava but I don't know what a timeline is / how concrete it is.

Fred

On Fri, Jul 20, 2012 at 6:37 PM, Leigh Klotz, Jr. <leigh...@gmail.com> wrote:

[Tim Peierls]

Another way to pull nontrivial initialization logic out of regular object graph creation is to use ConcurrentSingleton scope for heavyweight singletons, something I wrote about a while ago. It's similar in some ways to the idea of using Services to perform initialization.

--tim

[Leigh Klotz, Jr.]

Tim,

Thank you (as always) for the considerable thought on this.

On Friday, July 20, 2012 5:21:57 PM UTC-7, Tim Peierls wrote:

I didn't know about a Guice prohibition on doing things in constructors, unless it was to avoid problems with circular dependencies. (So don't have circular dependencies!)

I don't see a problem with Proposals A or D. I have yet to see it documented this way, but the Guice folks have assured me over the years that there is a happens-before edge between writes in @Inject-ed methods and reads of an injected object (unless you try very hard and do evil things during provision). Proposal A looks fine to me; Proposal D looks safe as long as the non-final Map is always accessed read-only after construction/injection (which seems to be the case).

OK, if these are both safe then I will probably stick with one of them until there's an annotation.

 

 

Proposals B and C seem unnecessarily verbose; the use of @Named to inject a Map<String, String> is overkill. If you want to use a @Provides method, isn't this sufficient? 

    @LazySingleton @Provides CachedThings cachedThings(DB db) {

        Map<String, String> cache = readCache(db);

        return new CachedThings(cache);

    }

This last seems like the nicest approach because you get to keep the cache field final, and you keep the reading separate from the access. "readCache" could be a static method of some other class in the same package, so that the Module's only role would be to say "To make the CachedThings from the DB, first read the cache from the DB into a map and construct the CachedThings from that map."

It's really clear what it does and it's fairly concise, but it fails to satisfy the case where CachedThings injects other values.  It also loses a bit on conciseness since the @Provides method is outside the definition of CachedThings.  I'd prefer to have this logic in one place.

 

But it really seems like it would be nice to have a class annotation to say "This class is a provider that makes a Singleton" or a Lazy Singleton, or a RequestScoped or for that matter any scope. 

Leigh.

--tim

To unsubscribe from this group, send email to google-guice+unsubscribe@googlegroups.com.

[Leigh Klotz, Jr.]

Fred,

Thanks for this answer.  I guess in a single-threaded scope such as @RequestScoped, it's safe to do the work on demand in the first access, but it's still clunky to write your own cache.

The multi-threaded laziness adds a level of complexity, and I was looking for a solution that hides that; your suggestion is to acknowledge it explicitly with a startup phase, which in fact is something I would do for this anyway; I just want the code to work correctly in case someone alters the graph to have dependencies between the services, which is of course what Tim's ConcurrentSingleton does.

The challenge is again to write this without splitting the creation of the Map<String,Integer> into a separate @Named construction, without doing the parsing in the constructor, and without doing the parsing in the accessor.

@RequestScoped

class Ratings {

   private final Map<String,Integer> ratings;

   @Inject Ratings(HttpServletRequest request) {

     this.ratings = request.parseRatings();  // fictitious bit of HTTP request metadata

   }

   public int getRating(String key) { 

     // alternatively, since @RequestScoped is single-threaded

     // if (ratings == null) ratings = request.parseRatings();

     return ratings.get(value); 

   }

}

Thank you,

Leigh.

Fred

To unsubscribe from this group, send email to google-guice+unsubscribe@googlegroups.com.

[Tim Peierls]

On Mon, Jul 23, 2012 at 12:25 PM, Leigh Klotz, Jr. <leigh...@gmail.com> wrote:

    @LazySingleton @Provides CachedThings cachedThings(DB db) {

        Map<String, String> cache = readCache(db);

        return new CachedThings(cache);

    }

This last seems like the nicest approach because you get to keep the cache field final, and you keep the reading separate from the access.

It's really clear what it does and it's fairly concise, but it fails to satisfy the case where CachedThings injects other values.  

Well, it could still be done:

@LazySingleton @Provides CachedThings cachedThings(DB db, MembersInjector<CachedThings> membersInjector) {

    Map<String, String> cache = readCache(db);

    CachedThings result = new CachedThings(cache);

    membersInjector.injectMembers(result);

    return result;

}

but as you say:

It also loses a bit on conciseness since the @Provides method is outside the definition of CachedThings.  I'd prefer to have this logic in one place.

So readCache in the constructor is probably the way to go for now. However:

I guess in a single-threaded scope such as @RequestScoped, it's safe to do the work on demand in the first access, but it's still clunky to write your own cache.

Is Guava's LoadingCache an option?

--tim