views:

94

answers:

5

There's a piece of code that looks like this. The problem is that during bootup, 2 initialization takes place. (1) Some method does a reflection on ForumRepository & performs a newInstance() purely to invoke #setCacheEngine. (2) Another method following that invokes #start(). I am noticing that the hashCode of the #cache member variable is different sometimes in some weird scenarios. Since only 1 piece of code invokes #setCacheEngine, how can the hashCode change during runtime (I am assuming that a different instance will have a different hashCode). Is there a bug here somewhere ?

public class ForumRepository implements Cacheable
{
    private static CacheEngine cache;
    private static ForumRepository instance;

    public void setCacheEngine(CacheEngine engine) { cache = engine; }

    public synchronized static void start()
    {
        instance = new ForumRepository();
    }

    public synchronized static void addForum( ... )
    {
        cache.add( .. );
        System.out.println( cache.hashCode() );
        // snipped
    }

    public synchronized static void getForum( ... )
    {
        ... cache.get( .. );
        System.out.println( cache.hashCode() );
        // snipped
    }
}

The whole system is wired up & initialized in the init method of a servlet. And the init() method looks like this conceptually:

// create an instance of the DefaultCacheEngine
cache = (CacheEngine)Class.forName( "com..DefaultCacheEngine" ).newInstance();
cache.init();

// init the ForumRepository's static member
Object o = Class.forName( "com.jforum....ForumRepository" ).newInstance();     
if( o instanceof Cacheable )
    ((Cacheable)o).setCacheEngine(cache);

// Now start the ForumRepository
ForumRepository.start();

UPDATE I didn't write this code. It is taken from jforum

UPDATE 2 Solution found. I added a separate comment below describing the cause of the problem. Thanks to everyone.

+1  A: 

You're going to have to give WAY more information than this, but CacheEngine is probably a mutable data type, and worse, it may even be shared by others. Depending on how CacheEngine defines its hashCode(), this could very well lead to aForumRepository seeing various different hash codes from its cache.

It's perfectly fine for the same object, if it's mutable, to change its hashCode() over a period of time, as long as it's done in a consistent manner (which is another topic altogether).

See also


On cache being static

More information has resurfaced, and we now know that the object in question, while mutable, does not @Override hashCode(). However, there seems to be a serious issue in design in making cache a static field of ForumRepository class, with a non-static "setter" setCacheEngine (which looks to be specified by Cacheable).

This means that there is only incarnation of cache, no matter how many ForumRepository instances are created! In a way, all instances of ForumRepository are "sharing" the same cache!

JLS 8.3.1.1 static Fields

If a field is declared static, there exists exactly one incarnation of the field, no matter how many instances (possibly zero) of the class may eventually be created. A static field, sometimes called a class variable, is incarnated when the class is initialized.

I think it's important to step back right now and ask these questions:

  • Does cache need to be static? Is this intended?
    • Should instances of ForumRepository have their own cache?
    • ... or should they all "share" the same cache?
  • How many instances of ForumRepository will be created?
    • Putting pros and cons of the design pattern aside, should ForumRepository be a singleton?
  • How many times can setCacheEngine be called on a ForumRepository object?
    • Would it benefit from a defensive mechanism of throwing IllegalStateException if it's called more than once?

The best recommendations would depend on the answers to the above questions. The third bullet point is something that is immediately actionable, and would reveal if setCacheEngine is getting invoked multiple times. Even if they're only invoked once for each ForumRepository instance, it's still effectively a multiple "set" in the current state of affairss, since there's only one cache.

A static field with a non-static setter is a design decision that needs to be thoroughly reexamined.

polygenelubricants
Yes CacheEngine is mutable. It contains an internal HashMap.
Jacques René Mesrine
I'm marking your answer as the best one mainly because you've given me ideas that have led to the solution.
Jacques René Mesrine
A: 

are you sure the ForumRepository classes that you are comparing are the same. if you are doing newInstance you might have a weird classloader issue.

drscroogemcduck
A: 

Mutable objects with hashCode implementations based on mutable state are almost always a bad idea. For example, they behave very strangely in hash-based collections -- if you insert such an object into a HashSet and then mutate it, the contains method won't be able to find it anymore.

Objects that are naturally distinguished by their identity should not override equals and hashCode at all. If you override hashCode based on state, that state should be immutable. Examples are String and the boxing types. Those are often referred to as "value classes", because their identity has no significance -- it is meaningless to distinguish between multiple instances of new Integer(42).

FredOverflow
A: 

The thing that puzzles me about this question is this:

Why are you looking at the hashcode of a CacheEngine instance?

It seems that your code is putting Forum objects into a cache and getting them back. As such, it makes sense to look at the hashcode values for the Forum instances. But the hashcode of the cache itself would appear to be entirely irrelevant.

Having said that, if the DefaultCacheEngine class inherits its implementation of hashcode from java.lang.Object then the value returned by the method is the "identity" hashcode, and this cannot change over the lifetime of an object. If it does appear to change, this means that you are now looking at a different instance of the DefaultCacheEngine class.

Stephen C
I printed out a value in the cache in totally different methods and the contents was different (i.e. they were missing newly created items). So that's why I went down the path of suspecting that the CacheEngine instance is being mutated or reinitialized. It certainly could be other things.
Jacques René Mesrine
A: 

I've solved my problem and I would like to share with you what I've learnt or discovered.

Root cause of the bug

The jforum.war webapp was being loaded twice by Tomcat 6.x, via two different virtual hosts. So yes, the CacheEngine was displaying two different hashCodes because they were loaded up in separate webapp classloaders.

Solution

The quick fix for me was to limit the invocation of the servlets in jforum.war via one specific virtual host address.

Jacques René Mesrine