Is cyclic dependency between anonymous class and parent class wrong?

Question

I have following snippet of code:

public class Example {  private Integer threshold;  private Map<String, Progress> history;  protected void activate(ComponentContext ctx) {     this.history = Collections.synchronizedMap(new LinkedHashMap<String, Progress>() {         @Override         protected boolean removeEldestEntry(Map.Entry<String, Progress> entry) {             return size() > threshold;         }     });   } } 

Theres is a cyclic dependency between anonymous LinkedHashMap class and Example class. Is this OK or not? Why not? Is it going to be nicely reclaimed by garbage collector?

Answer 1

Is this OK or not?

This is completely fine.

threshold is a field, so it can be referenced from within an anonymous class without any problem. (Had threshold been a local variable, it would have had to be (effectively) final.)

Cyclic dependencies between classes are common and when the dependency graph is small (as in this case) it doesn't pose any problems. The fact that your LinkedHashMap is an anonymous class doesn't matter here.

Is it going to be nicely reclaimed by garbage collector?

The only thing to be wary about regarding memory leaks + inner classes is that a (non-static) inner class has an implicit reference to its enclosing object. This means that if you create lots and lots of instances of the inner class, you can't expect the instances of the outer class objects to be garbage collected.

What that means in this case is that if you leak references to the history map, instances of Example will not be GC'ed.

---

Related notes:

• Considering you're using synchronizedMap it seems like you're working on a multithreaded program. If this is the case, you need to be wary about synchronization and visibility issues for the threshold field.

• If possible, try to make the threshold field final

• Another option would be to create a named class for your LinkedHashMap and include threshold as a field in that class instead.

Answer 2

You have this dependency anyway, because every object of anonymous inner class has implicit reference to the object of an enclosing class. Java is designed that way, and the nested inner classes have this reference for a reason, so from the language spec standpoint this compiles and looks perfectly normal.

Regarding the (absence of) "design smell", if this anonymous class object is completely encapsulated in Example class, has no distinctive meaning without its enclosing context, and is not leaked anywhere outside of the Example class, there is nothing wrong with referencing fields of enclosing class. You simply use this inner class to group some logic.

If however this object gets leaked out of the enclosing object (you return it via getter, for example), you should either prohibit this or refactor it into a static inner class that receives threshold as a parameter. This inner object holds reference to the enclosing object and may keep it from GC, thus causing a memory leak.