CODEWITHSUNDEEP
javaanonymous-classanonymous-inner-classcyclic-reference
Michal Chudy
Michal Chudy

Reputation: 1893

Is cyclic dependency between anonymous class and parent class wrong?

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?

Upvotes: 25

Views: 2048

Answers (5)

aioobe
aioobe

Reputation: 421160

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.

Upvotes: 22

Max
Max

Reputation: 539

I do not like your solution (even if I agree this could work) :

  1. your class Example should implement Map or extend LinkedHashMap because the instance variable threshold is defined there and refines the concept of LinkedHashMap with its own definition.

  2. your class Example should NOT implement Map or extend LinkedHashMap because the activate method does not refines LinkedHashMap nor Map but uses concepts of Maps.

1+2 => problem of conception.

Upvotes: 1

John
John

Reputation: 5297

Cyclic dependency is not bad in it's own, however it may cause some unsuspected memory leaks.

Taken your example as is, it is fine right now as it does what you want it to do.

If you however, or somebody else modifies your code to expose your private: 

private Map<String, Progress> history;

Then you may have trouble. What will happen is that you will pass around reference to Example class too, intended or not, as your inner class has implicit reference to it.

I cannot give you direct quote right now, but Steve McConnell in his code complete is calling the cyclic dependencies an anti-pattern. You can read there or i guess google for it, to read about this in great detail.

Another problem i can think of on top of my mind, cyclic dependency is fairly hard to unit test as you are creating very high level of coupling between objects.

In general, you should avoid circular dependency unless you have very good reason not to, such as implementing the circular linked list.

Upvotes: 2

Vasiliy
Vasiliy

Reputation: 16268

Any time you instantiate a non-static inner class (be it named or anonymous), this inner class' instance automatically gets a reference to the instance of the enclosing parent class.

The above means that if the outer class also holds a reference to the non-static inner class (as is the case in your code), then there is a cyclic dependency between instances of outer class and the non-static inner class (again, both named and anonymous).

The only actual question in this setup is whether your usage of this existing cross reference is legitimate. In your specific case, I don't see any issue - non-static inner class uses an instance variable of the enclosing outer class. Seems Kosher to me.

In this situation, memory leak usually happens when a reference to the instance of the inner class is passed outside of the outer class (which is commonly the case with various Listeners) - since this instance has a reference to the instance of the outer class, the outer class can't be garbage collected. However, I don't think a memory leak can be caused if you just cross reference the outer and the inner classes - they will be garbage collected together.

Upvotes: 2

Forketyfork
Forketyfork

Reputation: 7810

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.

Upvotes: 6

Related Questions