Deadlock using FactoryModuleBuilder from different threads

[GoogleCodeExporter]

As per the discussion here:

https://groups.google.com/forum/?fromgroups=#!topic/google-guice/cOaZVIXo5cI

We're experiencing a deadlock in our multi-threaded code, and I think I've 
tracked it down to our use of FactoryModuleBuilder to create one of our objects 
during an HTTP response. We use the pattern of:
install(new FactoryModuleBuilder()
                .implement(User.class, User.class)
                .build(UserFactory.class));

And our dead lock looks like (we have more than a dozen threads all blocked in 
the same way):
"Thread-2616" prio=10 tid=0x00007f41647e1800 nid=0xea9 waiting for monitor 
entry [0x00007f417fefd000]
   java.lang.Thread.State: BLOCKED (on object monitor)
        at com.google.inject.internal.InternalInjectorCreator.build(InternalInjectorCreator.java:102)
        - waiting to lock <0x0000000608d609a0> (a com.google.inject.internal.InheritingState)
        at com.google.inject.internal.InjectorImpl.createChildInjector(InjectorImpl.java:217)
        at com.google.inject.internal.InjectorImpl.createChildInjector(InjectorImpl.java:224)
        at com.google.inject.assistedinject.FactoryProvider2.getBindingFromNewInjector(FactoryProvider2.java:602)
        at com.google.inject.assistedinject.FactoryProvider2.invoke(FactoryProvider2.java:625)
        at $Proxy16.create(Unknown Source)
        at ourCode

So I am concluding that the Factory Module Builder pattern isn't thread safe 
per-se, and will just go back to creating our object manually, then using 
injector#injectMembers() afterwards.

Am I missing something or does this sound about right? Is this correct? Or am I 
missing something? If it is, it would be great to get a warning added 
somewhere..

The relavent Guice code is FactoryProvider2:
 /**
   * When a factory method is invoked, we create a child injector that binds all parameters, then
   * use that to get an instance of the return type.
   */
  public Object invoke(Object proxy, final Method method, final Object[] args) throws Throwable {

and then the synchro block in InternalInjectorCreator:

  public Injector build() {
    if (shellBuilder == null) {
      throw new AssertionError("Already built, builders are not reusable.");
    }

    // Synchronize while we're building up the bindings and other injector state. This ensures that
    // the JIT bindings in the parent injector don't change while we're being built
    synchronized (shellBuilder.lock()) {
      shells = shellBuilder.build(initializer, bindingData, stopwatch, errors);
      stopwatch.resetAndLog("Injector construction");

      initializeStatically();
    }

    injectDynamically();

    if (shellBuilder.getStage() == Stage.TOOL) {
      // wrap the primaryInjector in a ToolStageInjector
      // to prevent non-tool-friendy methods from being called.
      return new ToolStageInjector(primaryInjector());
    } else {
      return primaryInjector();
    }
  }

The entire relevant stack trace is attached (all blocked threads in Guice 
stack).

There is one running thread in the Guice stack, but we're definitely seeing 
either a total dead lock, or that it's taking longer than 10seconds to inject 
the classes.

Original issue reported on code.google.com by antony.s...@gmail.com on 24 Sep 2012 at 4:48

Attachments:

• tdump.log

[GoogleCodeExporter]

Do you open new threads from injected cxtors (or in your Providers)?  That can 
introduce potential deadlock.  There's nothing about assisted-inject that I 
know of that would make it deadlock in standard usage.

Do you have the stack trace of the thread holding the InheritingState lock (as 
opposed to waiting to hold it)?

Original comment by sberlin on 24 Sep 2012 at 5:11

[GoogleCodeExporter]

What's an "injected cxtors"? 

None of our modules have any threading code, nor our constructor methods.

Ah, I think that is this thread?
"Thread-2610" prio=10 tid=0x00007f416430f000 nid=0xea3 runnable 
[0x00007f416b9f8000]
   java.lang.Thread.State: RUNNABLE
    at java.lang.Class.getConstantPool(Native Method)
    at java.lang.System$2.getConstantPool(System.java:1130)
    at java.lang.reflect.Method.declaredAnnotations(Method.java:693)
    - locked <0x0000000775db9c60> (a java.lang.reflect.Method)
    at java.lang.reflect.Method.getAnnotation(Method.java:679)
    at com.google.inject.spi.InjectionPoint.getAtInject(InjectionPoint.java:466)
    at com.google.inject.spi.InjectionPoint.getInjectionPoints(InjectionPoint.java:664)
    at com.google.inject.spi.InjectionPoint.forInstanceMethodsAndFields(InjectionPoint.java:356)
    at com.google.inject.internal.MembersInjectorStore.createWithListeners(MembersInjectorStore.java:90)
    at com.google.inject.internal.MembersInjectorStore.access$000(MembersInjectorStore.java:34)
    at com.google.inject.internal.MembersInjectorStore$1.create(MembersInjectorStore.java:42)
    at com.google.inject.internal.MembersInjectorStore$1.create(MembersInjectorStore.java:39)
    at com.google.inject.internal.FailableCache$1.apply(FailableCache.java:39)
    at com.google.inject.internal.util.$MapMaker$StrategyImpl.compute(MapMaker.java:549)
    at com.google.inject.internal.util.$MapMaker$StrategyImpl.compute(MapMaker.java:419)
    at com.google.inject.internal.util.$CustomConcurrentHashMap$ComputingImpl.get(CustomConcurrentHashMap.java:2041)
    at com.google.inject.internal.FailableCache.get(FailableCache.java:50)
    at com.google.inject.internal.MembersInjectorStore.get(MembersInjectorStore.java:65)
    at com.google.inject.internal.ConstructorInjectorStore.createConstructor(ConstructorInjectorStore.java:73)
    at com.google.inject.internal.ConstructorInjectorStore.access$000(ConstructorInjectorStore.java:28)
    at com.google.inject.internal.ConstructorInjectorStore$1.create(ConstructorInjectorStore.java:36)
    at com.google.inject.internal.ConstructorInjectorStore$1.create(ConstructorInjectorStore.java:32)
    at com.google.inject.internal.FailableCache$1.apply(FailableCache.java:39)
    at com.google.inject.internal.util.$MapMaker$StrategyImpl.compute(MapMaker.java:549)
    at com.google.inject.internal.util.$MapMaker$StrategyImpl.compute(MapMaker.java:419)
    at com.google.inject.internal.util.$CustomConcurrentHashMap$ComputingImpl.get(CustomConcurrentHashMap.java:2041)
    at com.google.inject.internal.FailableCache.get(FailableCache.java:50)
    at com.google.inject.internal.ConstructorInjectorStore.get(ConstructorInjectorStore.java:49)
    at com.google.inject.internal.ConstructorBindingImpl.initialize(ConstructorBindingImpl.java:125)
    at com.google.inject.internal.InjectorImpl.initializeBinding(InjectorImpl.java:507)
    at com.google.inject.internal.AbstractBindingProcessor$Processor$1.run(AbstractBindingProcessor.java:159)
    at com.google.inject.internal.ProcessedBindingData.initializeBindings(ProcessedBindingData.java:44)
    at com.google.inject.internal.InternalInjectorCreator.initializeStatically(InternalInjectorCreator.java:122)
    at com.google.inject.internal.InternalInjectorCreator.build(InternalInjectorCreator.java:106)
    - locked <0x0000000608d609a0> (a com.google.inject.internal.InheritingState)
    at com.google.inject.internal.InjectorImpl.createChildInjector(InjectorImpl.java:217)
    at com.google.inject.internal.InjectorImpl.createChildInjector(InjectorImpl.java:224)
    at com.google.inject.assistedinject.FactoryProvider2.getBindingFromNewInjector(FactoryProvider2.java:602)
    at com.google.inject.assistedinject.FactoryProvider2.invoke(FactoryProvider2.java:625)
    at $Proxy16.create(Unknown Source)
    at ourcode

That's the only one that's running, within Guice, and has gone through the 
FP2#invoke

Original comment by antony.s...@gmail.com on 24 Sep 2012 at 5:14

[GoogleCodeExporter]

Thread-2610 is definitely runnable and will eventually release the 
InheritingState lock, so this isn't a deadlock situation - however the full 
thread dump does show this lock is a bottleneck in your application. Can you 
give more details about the User class, does it inject the injector or any 
providers into its constructor? (This disables certain optimizations in the 
AssistedInject code.)

Original comment by mccu...@gmail.com on 24 Sep 2012 at 5:18

[GoogleCodeExporter]

I'm really shocked if this is a bottle neck and not a dead lock issue as i 
wouldn't have thought our load was that high, or that creating this injector 
would take *that* long that it would cause our responses to start timing out.

Yes, it injects Injector, but no providers. It's field level injection. 

Original comment by antony.s...@gmail.com on 24 Sep 2012 at 5:23

[GoogleCodeExporter]

Assisted-Injector can be very slow if the assisted object injects the Injector 
(or Providers), because for every object creation it will need to create a 
child injector (which, as you can see, isn't the fastest of operations).  If 
you can refactor your User class to not require the Injector (ie, only take the 
dependencies it needs), this will speed things up massively.  See 
http://code.google.com/p/google-guice/issues/detail?id=435 for details.

Original comment by sberlin on 24 Sep 2012 at 5:45

[GoogleCodeExporter]

Ok, I guess we'll chalk this up to just "very slow" not dead lock, due to that 
single running thread.

We already refactored to remove the assisted inject, but we'll try it just 
without the injector first, and see what happens. (Turns out we weren't even 
using it anymore).

Is using assisted with a factory like this at all slow, compared to 
injector#injectMembers? If it is, I'd rather just refactor and remove it 
entirely.

Original comment by antony.s...@gmail.com on 24 Sep 2012 at 5:52

[GoogleCodeExporter]

If the assisted object doesn't inject an Injector nor Providers, assisted 
inject is a blip on the radar (comparable to a normal injection).

Original comment by sberlin on 24 Sep 2012 at 5:55

• Changed state: WorkingAsIntended

[GoogleCodeExporter]

Thanks for the timely help!

Original comment by antony.s...@gmail.com on 24 Sep 2012 at 5:59