TypeLiteral configured in child Injector leaked into parent Injector

[moreginger]

Perhaps this behavior is known but I couldn't find any mention of it. This code...

 public static void main(final String[] args) {
    Injector parent = Guice.createInjector();

    parent.createChildInjector(createModule(new GiantObject("notleaked")));

    // GiantObject("notleaked") can be garbage collected.

    parent.createChildInjector(createTypeLiteralModule(new GiantObject("leaked")));

    // GiantObject("leaked") cannot be garbage collected.
  }

  private static AbstractModule createModule(final GiantObject o) {
    return new AbstractModule() {
      @Override
      protected void configure() {
        bind(GiantObject.class).toInstance(o);
      }
    };
  }

  private static AbstractModule createTypeLiteralModule(final GiantObject o) {
    return new AbstractModule() {
      @Override
      protected void configure() {
        bind(new TypeLiteral<GiantObject>() {
        }).toInstance(o);
      }
    };
  }

...puts a reference to the TypeLiteral in the parent Injector, and appears never to be garbage collected. JProfiler lists InheritingState/WeakKeySet in the first GC root. This leaks whatever is in scope for the TypeLiteral (i.e. the GiantObject("leaked") instance).

[moreginger]

JProfiler object graph to GC root.

[sameb]

Could you write this as a junit testcase where there's two sibling child injectors that attempt to bind(new TypeLiteral<Foo>() {}).toInstance(fooInstance); ? If the binding is being promoted to the parent then the second bind should fail. If the binding is in the child injector, then the test would pass.

[sameb]

Also: what version of Guice are you using?

[moreginger]

Oh sorry, this is guice-4.0-beta5. It is also an internal repackaged version. I'll double check with pure guice.

As for the JUnit test, I don't think the binding is being promoted, it just remembers the TypeLiteral itself for some reason and (since it is an anonymous inner class) that causes the leak.

[sameb]

Here's what's happening:

1. You construct GiantObject("leaked") and pass it to a method.
2. GiantObject is a final var for GuiceTest$2 (the module in createTypeLiteralModule), so the anonymous module class retains a reference to o (the GiantObject).
3. You construct a TypeLiteral as an anonymous class (GuiceTest$2$1), which retains a reference to its enclosing class (GuiceTest$2), which in turn retains a reference to o (the GiantObject).

All this is retained through WeakKeySet's cache, associated to the child injector's state.

Fortunately, AFAICT, this is all working as designed. Unless something else retains a reference to the child injector's state, then when GC happens the state will be cleaned up and everything will get discarded.

Do you have reason to believe the state is being held by a strong ref & isn't being GC'd? If so, that'd be a problem.

[moreginger]

Thanks very much for the feedback. I do find it unlikely that Guice has a bug here, but I can't work out what I've done wrong.

OK I tried it with <dependency org="com.google.inject" name="guice" rev="4.0-beta5"/> and I still got the leak.

How about the following. testBindToClass passes and testBindToTypeLiteral throws OutOfMemoryError for me, when run with -Xmx32m...

import org.junit.Test;

import com.google.inject.AbstractModule;
import com.google.inject.Guice;
import com.google.inject.Injector;
import com.google.inject.TypeLiteral;

public class GuiceTest {

  public static class GiantObject {
    private final String _name;

    private final Object[] _array;

    public GiantObject(final String name) {
      _name = name;
      _array = new Object[5000000];
    }
  }

  @Test
  public void testBindToClass() {
    Injector parent = Guice.createInjector();
    parent.createChildInjector(createModule(new GiantObject("notleaked")));
    Object[] array = new Object[5000000];
  }

  @Test
  public void testBindToTypeLiteral() {
    Injector parent = Guice.createInjector();
    parent.createChildInjector(createTypeLiteralModule(new GiantObject("leaked")));
    Object[] array = new Object[5000000];
  }

  private static AbstractModule createModule(final GiantObject o) {
    return new AbstractModule() {
      @Override
      protected void configure() {
        bind(GiantObject.class).toInstance(o);
      }
    };
  }

  private static AbstractModule createTypeLiteralModule(final GiantObject o) {
    return new AbstractModule() {
      @Override
      protected void configure() {
        bind(new TypeLiteral<GiantObject>() {
        }).toInstance(o);
      }
    };
  }

}

[sameb]

That does look suspect. Can you use a memory analyzer to try to find different roots holding the child injector's state or the GiantObject?

[moreginger]

According to JProfiler, the path in is the only path to a GC root.

[sameb]

I'm at a loss then. Would you be able to dig into who might be holding the child injector's InternalState object?

[sameb]

Oh, wait, I know (I think). We don't eagerly clean up the WeakKeySet cache. You have to access or mutate it in some way to trigger it to cleanup. What happens if you do a second call to injector.createChildInjector before creating the second big array?

[moreginger]

That sounds compelling, but...

  @Test
  public void testBindToTypeLiteral() {
    Injector parent = Guice.createInjector();
    parent.createChildInjector(createTypeLiteralModule(new GiantObject("leaked", 5000000)));
    parent.createChildInjector(new AbstractModule() {
      @Override
      protected void configure() {
        bind(new TypeLiteral<GiantObject>() {
        }).toInstance(new GiantObject("hmmm", 1));
      }
    });
    Object[] array = new Object[5000000];
  }

...doesn't help matters (integer is size of Object[] so that I could create the second instance to try and remove the TypeLiteral). Perhaps triggering cleanup requires additional steps?

[moreginger]

OK, had a look at the Guice code.

AFAICT WeakKeySet.evictionCache.cleanUp is only called when a JustInTimeBinding is needed. So for example this modified test passes for me with -Xmx32m:

  @Test
  public void testBindToTypeLiteral_evictionCacheCleanUp() {
    Injector parent = Guice.createInjector();
    parent.createChildInjector(createTypeLiteralModule(new GiantObject("leaked")));
    System.gc(); // Need to provoke gc to get child injector disposed
    parent.getInstance(String.class); // Cause a JustInTimeBinding to be created so that evictionCache.cleanUp is invoked
    Object[] array = new Object[5000000];
  }

So it seems like there's a genuine leak here that has the potential to hang around indefinitely (i.e. until a JustInTime binding is required).
I unfortunately don't have the resources to investigate a fix. Perhaps it would be possible to clean up the eviction cache more aggressively?

[sameb]

Thanks for taking a closer look, @moreginger! I'll see if there's something more we can do here.

[sameb]

I did some work in 825f8c1 which should alleviate the problem. It doesn't do any aggressive cleaning of WeakKeySet, but it does make sure that Key/TypeLiteral don't retain references to their surrounding classes, so the WeakKeySet shouldn't ever hold onto more data. When I merge #912, I'll close this since I think that's the best we can do. (I looked into more aggressive cleanup, but there's no good way I could find to do it that wouldn't add a lot of overhead to critical fast paths.)

[sameb]

Should be fixed in the latest snapshot. Can you give it a go (once it auto-publishes) and see how it works?

[moreginger]

Pulled latest code and that works for me, thanks :)
I don't think latest snapshot is new enough yet.