How to implement concurrency in Queue

Question

I am trying to implement a queue which should be synchronized. The requirement is:

If the Queue is empty and new data comes then data should be inserted in queue and data processing should start as long as queue does not become empty.
If Queue is not empty, that means the data in the queue is still being processed. So if some new data comes then the data should just be added to queue.

Here is my implementation:

class QueueManager {
        private var jobsQueue: LinkedList<String> = LinkedList()
        fun handleIncomingJob() {
          if (isEmpty()) {
             addJ(jobList)
             start()
          } else {
             addJ(jobList)
          }
       }

private fun isEmpty(): Boolean {
        synchronized(this@QueueManager) {
            val ret = jobsQueue.isEmpty()
            return ret
        }
    }

    private fun addJ(jobList: Array<SAPJob>) {
        synchronized(this@QueueManager) {
            jobsQueue.add(jobList)
        }
    }

    private fun remove(): Array<SAPJob> {
        synchronized(this@QueueManager) {
            return jobsQueue.remove()
        }
    }

       fun start() {
            while (!isEmpty()) {
                Thread.sleep(10000)
                remove()
            }
       }
}

But the above code does not work. It fails when multiple thread call handleIncomingJob method at once and all of them find the queue to be empty. As per my requirement only the first thread should find the queue empty, add data to it and start processing . And the all the other threads should find that since first thread put item in the queue so its not empty , so they should just put the data to queue and exit. But as per above implementation all thread find the queue to be empty. I do not know whats the problem. I have made all the queue methods to be synchronized. Still it does not work

score 1 · Accepted Answer · answered Sep 13 '19 at 23:10

Your synchronization is local to multiple functions yet two or more of those functions make up a single critical section. In other words, you don't hold the lock across function calls which means threads can read inconsistent state. For instance, the following can happen inside the processIncomingJob() function:

Thread A calls isEmpty() and the result is true.
Thread A moves into if block.
Before thread A can call addJ(...) another thread, B, calls isEmpty() and because thread A has yet to actually add the job, the result is also true.
Thread B moves into if block.
Eventually both thread A and thread B call the start() function, leading to your problem.

When making your code thread-safe you have to encapsulate the entire unit-of-work in a synchronized context. What a "unit-of-work" actually comprises is entirely dependent on the business logic of your code. Note your start() function suffers from the same problem.

Your code example also has other problems:

Your jobsQueue is defined to be a LinkedList<String> yet everywhere else it seems to be treated as a LinkedList<Array<SAPJob>>.
What is SAPJob?
What is jobList in the context of handleIncomingJob()?
Inside start() you call remove() but do nothing with the result.
Why is start() public?

Due to these problems it's not easy to completely understand what you want to happen in your code. However, based on your descriptions of both the intent and problem, I believe the following example may help you find a solution for your use case:

import java.util.LinkedList
import java.util.Queue

typealias Job = () -> Unit

class JobProcessor {

    private val lock = Any()
    private val queue: Queue<Job> = LinkedList()
    private var isExecuting = false

    fun processJob(job: Job) {
        val execute = synchronized(lock) {
            queue.add(job)
            !isExecuting.also { isExecuting = true }
        }
        if (execute) {
            executeQueuedJobs()
        }
    }

    private fun executeQueuedJobs() {
        var keepExecuting: Boolean
        do {
            val job = synchronized(lock) { queue.remove() }
            try {
                job()
            } catch (ex: Exception) {
                Thread.currentThread().uncaughtExceptionHandler
                    .uncaughtException(Thread.currentThread(), ex)
            }
            keepExecuting = synchronized(lock) {
                queue.isNotEmpty().also { isExecuting = it }
            }
        } while (keepExecuting)
    }

}

When a job is added to the queue, the thread adding the job will only execute the job is no other thread is currently executing previous jobs. The thread executing the jobs will continue to do so until the queue is empty after the current job is complete. Note that a job is executed without holding the lock in order to allow other jobs to be queued while one job is executing. The purpose of isExecuting is to handle the case where a thread is executing a job but the queue is empty.

How to implement concurrency in Queue

1 Answers1