2
\$\begingroup\$

I had a task to realize ReadWriteMap

public class ReadWriteMap implements Map {

    private HashMap map = new HashMap();
    private ReadWriteLock readWriteLock = new ReentrantReadWriteLock();
    private Lock readLock = readWriteLock.readLock();
    private Lock writeLock = readWriteLock.writeLock();

    @Override
    public int size() {
        try (AutoCloseableLockWrapper autoCloseableLockWrapper = new AutoCloseableLockWrapper(readLock)) {
            autoCloseableLockWrapper.lock();
            return map.size();
        }

    }

    @Override
    public boolean isEmpty() {
        try (AutoCloseableLockWrapper autoCloseableLockWrapper = new AutoCloseableLockWrapper(readLock)) {
            autoCloseableLockWrapper.lock();
            return map.isEmpty();
        }
    }

    @Override
    public boolean containsKey(Object key) {
        try (AutoCloseableLockWrapper autoCloseableLockWrapper = new AutoCloseableLockWrapper(readLock)) {
            autoCloseableLockWrapper.lock();
            return map.containsKey(key);
        }
    }

    @Override
    public boolean containsValue(Object value) {
        try (AutoCloseableLockWrapper autoCloseableLockWrapper = new AutoCloseableLockWrapper(readLock)) {
            autoCloseableLockWrapper.lock();
            return map.containsKey(value);
        }
    }

    @Override
    public Object get(Object key) {
        try (AutoCloseableLockWrapper autoCloseableLockWrapper = new AutoCloseableLockWrapper(readLock)) {
            autoCloseableLockWrapper.lock();
            return map.get(key);
        }
    }

    @Override
    public Object put(Object key, Object value) {
        try (AutoCloseableLockWrapper autoCloseableLockWrapper = new AutoCloseableLockWrapper(writeLock)) {
            autoCloseableLockWrapper.lock();
            return map.put(key, value);
        }
    }

    @Override
    public Object remove(Object key) {
        try (AutoCloseableLockWrapper autoCloseableLockWrapper = new AutoCloseableLockWrapper(writeLock)) {
            autoCloseableLockWrapper.lock();
            return map.remove(key);
        }
    }

    @Override
    public void putAll(Map m) {
        try (AutoCloseableLockWrapper autoCloseableLockWrapper = new AutoCloseableLockWrapper(writeLock)) {
            autoCloseableLockWrapper.lock();
            map.putAll(m);
        }

    }

    @Override
    public void clear() {
        try (AutoCloseableLockWrapper autoCloseableLockWrapper = new AutoCloseableLockWrapper(writeLock)) {
            autoCloseableLockWrapper.lock();
            map.clear();
        }

    }

    @Override
    public Set keySet() {
        try (AutoCloseableLockWrapper autoCloseableLockWrapper = new AutoCloseableLockWrapper(readLock)) {
            autoCloseableLockWrapper.lock();
            return map.keySet();
        }
    }

    @Override
    public Collection values() {
        try (AutoCloseableLockWrapper autoCloseableLockWrapper = new AutoCloseableLockWrapper(readLock)) {
            autoCloseableLockWrapper.lock();
            return map.values();
        }
    }

    @Override
    public Set<Entry> entrySet() {
        try (AutoCloseableLockWrapper autoCloseableLockWrapper = new AutoCloseableLockWrapper(readLock)) {
            autoCloseableLockWrapper.lock();
            return null;
        }
    }
}

and lock wrapper:

public class AutoCloseableLockWrapper implements AutoCloseable, Lock{
    private final Lock lock;
    public AutoCloseableLockWrapper(Lock l) {
        this.lock = l;
    }
    @Override
    public void lock() {
        this.lock.lock();
    }

    @Override
    public void lockInterruptibly() throws InterruptedException {
        lock.lockInterruptibly();
    }

    @Override
    public boolean tryLock() {
        return lock.tryLock();
    }

    @Override
    public boolean tryLock(long time, TimeUnit unit) throws InterruptedException {
        return lock.tryLock(time,unit);
    }

    @Override
    public void unlock() {
        lock.unlock();
    }

    @Override
    public Condition newCondition() {
        return lock.newCondition();
    }
    @Override
    public void close() {
        this.lock.unlock();
    }
}

I see a lot of similar code in ReadWriteMap, but I don't know how to rewrite it.

And I am not sure that every method creating its own lock wrapper is good idea. I don't know how to composite single lock instances and try with resources.

How do I improve my code?

\$\endgroup\$
2
\$\begingroup\$

You should also wrap the result of keySet, valueSet and entrySet so accesses to them are also protected by the locks. This cascades downwards to include the iterators, streams and entries. The only thing you should ever return bare from the wrapped map is the keys or values themselves.

Unlocking a LockWrapper or forgetting to lock it results in an error.

try (AutoCloseableLockWrapper autoCloseableLockWrapper = new AutoCloseableLockWrapper(readLock)) {
    //...
}//calls readLock.unlock when it's never been locked in the first place -> error

Keep track of the locked status in your LockWrapper and change close() to check that status:

private boolean = locked = false;

@Override
public void close() {
    if(this.locked)
        this.lock.unlock();
}

@Override
public void lock() {
    if(this.locked) 
        throw new IllegalMonitorStateException("lock is already locked");
    //guard against locking twice
    this.lock.lock();
    this.locked = true;
}

On that note I suggest renaming AutoCloseableLockWrapper to LockGuard and remove the implements Lock from it. Some added functionality for it would be adopting a locked lock, relinquishing a locked lock (without unlocking it).

I also expect (from experience with such LockGuards in C++) that creating one will automatically lock the lock for you. There is generally an overload for when you don't want that:

public LockGuard(Lock l) {
    this.lock = l;
    this.lock();
}

public LockGuard(Lock l, boolean autoLock) {
    this.lock = l;
    if(autoLock)
        this.lock();
}

Making these changes (and picking a shorter name for the guard instance) will result in methods that look like:

@Override
public boolean isEmpty() {
    try (LockGuard lg = new LockGuard(readLock)) {
        return map.isEmpty();
    }
}
\$\endgroup\$
  • \$\begingroup\$ I am not fully understood about You should also wrap the result of keySet, valueSet and entrySet \$\endgroup\$ – gstackoverflow Aug 6 '15 at 20:06
  • \$\begingroup\$ in your current implementation if I do rwMap.ketSet().clear(); I clear the entire map with only read-locking once which gets unlocked before the actual clear happens. \$\endgroup\$ – ratchet freak Aug 6 '15 at 20:44
  • \$\begingroup\$ I understood it. I have not ideas how to prvent this \$\endgroup\$ – gstackoverflow Aug 6 '15 at 21:10
  • \$\begingroup\$ Create a ReadWriteSet and ReadWriteCollection that does what your ReadWriteMap does. Add a package private constructor that takes the ReadWriteLock to use for synchronization. \$\endgroup\$ – ratchet freak Aug 6 '15 at 21:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.