Is a EJB Singleton synchronized?

Question

I have implemented a kind of caching bean to cache data objects as an EJB Singleton. I wonder if this is the correct way in EJB:

@Singleton
public class MyCache {

    int DEFAULT_CACHE_SIZE = 30;
    int DEFAULT_EXPIRES_TIME = 60000;
    long expiresTime = 0;
    long lastReset = 0;
    Cache cache = null; 

    ....
    @PostConstruct
    void init() {
        resetCache();
    }

    public void resetCache() {
        cache = new Cache(DEFAULT_CACHE_SIZE);
        lastReset = System.currentTimeMillis();
    }

    public void put(String key, Object value) {
        cache.put(key, value);
    }

    public Object get(String key) {
        // test if cache is expired
        if (expiresTime > 0) {
            Long now = System.currentTimeMillis();
            if ((now - lastReset) > expiresTime) {
                logger.finest("...... Cache expired!");
                resetCache();
            }
        }
        return cache.get(key);
    }



    class Cache extends LinkedHashMap<String, Object> implements Serializable {
        private static final long serialVersionUID = 1L;
        private final int capacity;

        public Cache(int capacity) {
            super(capacity + 1, 1.1f, true);
            this.capacity = capacity;
        }

        protected boolean removeEldestEntry(Entry<String, Object> eldest) {
            return size() > capacity;
        }
    }
}

My question is: is this the right way to implement an application wide caching mechanism?

I have the impression that the contents of the cache are unexpectedly changing. Could this happen? For example, if the EJB is passivated? I am running in a Payara41 Server.

Or must I use:

cache = Collections.synchronizedMap(new Cache(DEFAULT_CACHE_SIZE));

instead of:

cache = new Cache(DEFAULT_CACHE_SIZE);

Answer 1

First of all, as concurrency management is not specified for your bean, it falls down to default "container".

From EJB 3.1 spec:

When designing a Singleton session bean, the developer must decide whether the bean will use container managed or bean managed concurrency. Typically Singleton beans will be specified to have container managed concurrency demarcation. This is the default if no concurrency management type is specified.

Then, container concurrency management needs method-level specifications of lock type. As soon as those are absent, the default "Write" applies:

By default, if a concurrency locking attribute annotation is not specified for a method of a Singleton bean with container managed concurrency demarcation, the value of the concurrency locking attribute for the method is defined to be Write.

The above means that access to your bean methods must be synchronized, probably even more than you actually need. You can set "Read" locking type for read-only methods (get), to allow concurrent read access.

Answer 2

The solution with container managed lock has one drawback, consider if you have a put operation with WRITE lock it means the whole "cache" is blocked so no get or put is possible in parallel, no matter whether the key is different. If your cache implementation is a concurrent Map you can use it without locking.

If you have more requirements for the cache I would use Infinispan which provide a better performance. The cache here can be local or distributed in a cluster.

Answer 3

After reading this blog from Adam Bien I now improved my code in the following way:

• I added the annotation ConcurrencyManagement
• I changed my LinkedHashMap into a ConcurrentHashMap.

Example:

@Singleton
@ConcurrencyManagement(ConcurrencyManagementType.BEAN) // added concurrency management
public class MyCache {

    int DEFAULT_CACHE_SIZE = 30;
    int DEFAULT_EXPIRES_TIME = 60000;
    long expiresTime = 0;
    long lastReset = 0;
    Cache cache = null; 

    ....
    @PostConstruct
    void init() {
        resetCache();
    }

    public void resetCache() {
        cache = new Cache(DEFAULT_CACHE_SIZE);
        lastReset = System.currentTimeMillis();
    }

    public void put(String key, Object value) {
        cache.put(key, value);
    }

    public Object get(String key) {
        // test if cache is expired
        if (expiresTime > 0) {
            Long now = System.currentTimeMillis();
            if ((now - lastReset) > expiresTime) {
                logger.finest("...... Cache expired!");
                resetCache();
            }
        }
        return cache.get(key);
    }

    // changed from LinkedHashMap to ConcurrentHashMap
    class Cache extends ConcurrentHashMap<String, Object> implements Serializable {
        private static final long serialVersionUID = 1L;
        private final int capacity;

        public Cache(int capacity) {
            super(capacity + 1, 1.1f);
            this.capacity = capacity;
        }

        protected boolean removeEldestEntry(Entry<String, Object> eldest) {
            return size() > capacity;
        }
    }
}

I think this is now the more correct implementation.