7

I have a main thread and a worker thread. The main thread adds tasks into a queue and the worker thread takes them to compute data. Before I put the objects into the queue I call lock on a ReentrantLock object (on the main thread) inside the task objects. When the worker thread is finished with working on a task from the queue I call unlock (on the worker thread). The problem is that I get an IllegalMonitorStateException because I call lock and unlock on different threads.

I am looking for an alternative lock system where I can do this on different threads.

Example:

public class Worker extends Thread {
    public static Queue<Task> tasks = new ConcurrentLinkedQueue<Task>();

    @Override
    public void run() {
        while (true) {
            Task task = tasks.poll();

            if (task != null) {
                task.work();
                task.lock.unlock(); // Here is the unlock, Task#i should not change up to now
            }
        }
    }
}


public class Task {
    private int i = 0;
    public Lock lock;

    public void setI(int i) {
        lock.lock();
        this.i = i;
        lock.unlock();
    }

    public void work() {
        System.out.println(i);
    }
}


public class Test {
    Task task = new Task();

    public void addTask() {
        task.lock.lock(); // Here is the lock, Task#i should not change
        Worker.tasks.add(task);
    }
}
stonar96
  • 1,359
  • 2
  • 11
  • 39
  • What are you _trying to_ protect with your lock? – Savior Apr 15 '16 at 16:28
  • @Pillar Data inside the objects. – stonar96 Apr 15 '16 at 16:29
  • synchronize on the object before locking & unlocking – ControlAltDel Apr 15 '16 at 16:29
  • Then lock around that. Your lock seems misplaced. – Savior Apr 15 '16 at 16:30
  • Can you post your code to understand the problem in better way? – Ravindra babu Apr 15 '16 at 16:31
  • I will post an example. – stonar96 Apr 15 '16 at 16:35
  • I am not entirely sure what you want to achieve. But you could lock before you queue and release after you have successfully added the item to the queue from the main thread. Likewise the worker thread should acquire the lock after it removes it from the queue and releases the lock after finishing work on it. Note that this process also requires a thread safe queue. Unless we can understand exactly what you seek to do we cant offer a solution. – MS Srikkanth Apr 15 '16 at 16:40
  • The main thread can modify fields inside the objects in the queue at any time. But it should not. The fields should not change after I added them to the queue and before the worker thread is not finished. That's why I lock when I add the objects and unlock when the worker thread is finished. – stonar96 Apr 15 '16 at 16:44
  • 3
    You can do what you want by using a `Semaphore` instead of a `ReentrantLock`, but locking in one thread and unlocking in another is _not_ a valid way to protect data. – Solomon Slow Apr 15 '16 at 16:45
  • @Stonar - if the fields should not change after you add the ifem to the queue and if the main thread is the only thread that edits it and it is also the same thread that queues it then I don't see why my solution in the comment above won't work – MS Srikkanth Apr 15 '16 at 16:48
  • Added example. @user3493289 The main thread adds a task to the queue and moves on working. And now the main thread is still able to modify fields with the setter method. But that should not happen until the worker thread is finished. – stonar96 Apr 15 '16 at 17:01
  • @stonar96 - won't a simple check like if item is in the queue don't edit it inside the main thread work? If it is not in the queue then the main thread can try acquiring the lock. If it is not able to acquire it then it means the worker thread is working on it. So it should wait or do something else. – MS Srikkanth Apr 15 '16 at 17:10
  • @user3493289 That would be slower more effort and maybe there would be still race conditions. I think a Semaphore is the right way to go. If someone can show me how to use it in my example I accept it. – stonar96 Apr 15 '16 at 17:21

3 Answers3

9

Why not use a Semaphore with just one permit? Instead of the lock operation you acquire the single permit. You should always free the lock with the release().

borjab
  • 11,149
  • 6
  • 71
  • 98
  • Is it that a Semaphore can be acquired in one thread and released in another while a lock must be locked/unlocked within the same thread? My application is like this: in thread 1 I run works with a infinite loop while(true){ ... theLock.lock(); //this is to make sure all work of the previous run has been finished then perform several pieces of works, all asynchronous in different threads, each of which will send a message to a master thread when done }, in thread 2 the work done messages are received and the lock is unlocked when all done. A Semaphore works but a ReentrantLock dose not. – Yulin Jan 05 '19 at 15:24
0

As per the question , It does not look right way to design multi-threaded application.

Either worker thread should handle creation of object or you should pass immutable object to worker thread and once worker thread is finished, it can pass result back to main thread.

I dont think it is poosible to acquire lock in one thread and unlock in another.

Dave
  • 35
  • 10
  • I can't redesign the application. I modify an application. The original application is single threaded and I want to add this worker thread which should do work with an object's state at the time it was added to the queue. – stonar96 Apr 15 '16 at 18:04
0

You don't need an alternative lock system. The ConcurrentLinkedQueue data structure already provides it's own lock system and guarantees thread safety. Any extra locking is unnecessary.

But you are re-inventing the wheel here. I'd recomment you take a look at ExecutorService and ThreadPools. While it is a good learning experience to build things yourself. It is also a big source of bugs.

ExecutorService workerPool = Executors.newFixedThreadPool(10); // 10 worker threads
...
Runnable myTask = ...;
workerPool.submit(myTask); // called from the main thread
...
Alen Vrečko
  • 886
  • 10
  • 13