3

I have structure something like this:
Lock wrapper - is used to store lock, condition and an object from response

public class LockWrapper{
    private Lock lock;
    private Condition myCondition;
    private MyObject myObject;

    public LockWrapper(Lock lock, Condition myCondition) {
        this.lock = lock;
        this.myCondition = myCondition;
    }

    public Condition getMyCondition() {
        return myCondition;
    }

    public MyObject getMyObject() {
        return myObject;
    }

    public void setObject(MyObject myObject) {
        this.myObject = myObject;
    }

    public Lock getLock() {
        return lock;
    }

}

Task - pushed into a thread pool for execution. It initiates requests to a server and then waits for server responses.

public class MyTask implements Runnable{
    private Lock lock = new ReentrantLock();
    private Condition myCondition = lock.newCondition();
    private MyWebSocketAPI api;

    public MyTask(MyWebSocketAPI api) {
         this.api = api;
    }

    @Override
    public void run() {
         lock.lock();
         try {
              // long randomLong = generateRandomLong();
              api.sendRequest(randomLong, new LockWrapper(lock, myCondition));
              myCondition.await();
              //do something after we got a response
         } finally{
              lock.unlock();
         }
    }

}

WebSocket - gets requests and notifies tasks about responses

 public abstract class MyWebSocketAPI extends WebSocketClient {
    //...
    private Map<Long, LockWrapper> lockWrappers = new ConcurrentHashMap<>();

    public void sendRequest(Long id, LockWrapper lockWrapper){
        this.lockWrappers.put(id, lockWrapper);
        //processRequest
    }

    @Override
    public void onMessage(String message) {
        LockWrapper lockWrapper = lockWrappers.get(message.get(0).getAsLong());

        lockWrapper.getLock().lock();
        try{
            lockWrapper.setMyObject(new MyObject(message));
            this.lockWrappers.put(message.get(0).getAsLong(), lockWrapper);
            lockWrapper.getMyCondition().signalAll();
        } finally {
            lockWrapper.getLock().unlock();
        }
    }

    //...
 }

Line lockWrapper.getMyCondition().signalAll(); throws an exception:

java.lang.IllegalMonitorStateException: null
    at java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject.signalAll(AbstractQueuedSynchronizer.java:1954)

Why my conditions throw this exception when I try to notify tasks that we got their objects? Did I make a mistake somewhere or Java doesn't allow shared conditions?

Oleksandr
  • 3,574
  • 8
  • 41
  • 78
  • @khelwood sorry, edited the code. Yes it is LockWrapper – Oleksandr Sep 12 '17 at 12:37
  • 1
    You need to hold the lock on myCondition before you can signal it. synchronized(myCondition) ... signalall() – JJF Sep 12 '17 at 12:42
  • @JJF Sorry, maybe I don't understand something in concurrency. Could you explain it a little bit more? I thought I already hold the lock in both cases. How correctly, then acquire the lock? – Oleksandr Sep 12 '17 at 12:49
  • One possible option for this is that somewhere you create `LockWrapper` with "wrong" args: condition which isn't owned by passed lock. You can change ctor to `LockWrapper(Lock lock)`, removing `Condition` arg, and create that condition inside `LockWrapper`. – Victor Sorokin Sep 12 '17 at 21:32
  • How can you do `message.get(0).getAsLong()` when `message` is a `String`? Post a [mcve] – Holger Sep 13 '17 at 11:47
  • Besides that, it looks highly suspicious that you perform `LockWrapper lockWrapper = lockWrappers.get(message.get(0).getAsLong());` at the beginning, but feel the need to do `this.lockWrappers.put(message.get(0).getAsLong(), lockWrapper);` later on. – Holger Sep 13 '17 at 11:56
  • Sorry for stupid mistake. I found it (posted as an answer). It was totally my stupid mistake. I even didn't think that I was creating two different locks and two different conditions. I was sure that the lock and condition are initialized only in one place. – Oleksandr Sep 13 '17 at 15:04

1 Answers1

0

It was my error in the Task. The problem was that I was creating Lock and Condition both global and local in method run. Lock and condition had the same name. In some cases I was using lock and in some cases this.lock (But it was two different locks). As a result, in method onMessage I had Condition and Lock which were not connected together. After I removed duplicates everything works.

Oleksandr
  • 3,574
  • 8
  • 41
  • 78