Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a DelayQueue in Java, which multiple threads will read from. They will then perform a task, and call a requeue method which performs some mathematical logic (no odd concurrent access), and then calls queue.put(this). I'm aware that both take and put are blocking methods on the queue, so will this risk any deadlocks?

Here's the processing code (to be run on 8 threads concurrently):

 Task task;
            try {
                task = queue.take();
            } catch (InterruptedException e) {
                logger.error("InterruptedException in futures processor trying to take from queue.");
                continue rLoop;
            }
           task.getCallback().run();
           task.requeue();

task.requeue(), again, makes a blocking call of queue.put() but the queue is unbounded and I'm not expecting any more than 10,000 actual queue elements over application lifetime, and no more than 100 or so at once.

share|improve this question
    
are your queue operations thread safe? –  Kevindra Mar 18 at 20:46
    
@KevindraSingh Yes, they are thread-safe, and block if there is no room to add an element for put, and will block if no elements exist for get. –  hexafraction Mar 18 at 20:49
add comment

2 Answers 2

up vote 4 down vote accepted

Why are you requeueing the instance instead of completing the operation...... ? Just asking...

If you really have to reque the instance, and if there is no other locking happening around the code you have shown, then there should be no problems with deadlocks.

Once you have taken the instance from the queue, your lock on the queue is relinqished. There is nothing you can do at this point to deadlock against anything else.

When you requeue the task, it is not going to deadlock either because it is not currently locking the queue.

So, you are fine.... technically.

Your code is a little awkard though. Why do you have part of your logic outside the try/catch? It would be neater if you put it inside the try/catch.

Also, your handling of InterruptedException is not great..... The InterruptedException is a real PITA, but you have to just deal with it. The minimum you can/should do is:

Thread.currentThread().interrupt();
share|improve this answer
    
As for the logic outside try/catch, that's an artifact of creating a smaller sample that illustrates the actual question at hand. The tip on InterruptedException is helpful, though. (Also, requeueing is necessary as the task may need to be repeated with a delay due to unacceptable data from a physical sensor, and the task's internal state will change if that is the case) –  hexafraction Mar 18 at 20:50
add comment

I noticed that DelayQueue is from the concurent package so in the put and take methods should not be a problem.

From this piece of code I believe you cannot arrive to a deadlock.

The reason is: you hold only maximum of one resource at any time (and never request to hold second one). So you dont have a circular dependence in a terms that "I am holding nail and requesting a hammer"

What would be worth checking is the logic that you have in requeue, but the presented piece of code looks fine.

Interesting link for deadlock possibilities: http://en.wikipedia.org/wiki/Deadlock#Necessary_condition

Your code does not fulfill (at least) the second "Hold and Wait" so it prelude the deadlock from happening, based on the definition. It does not fulfill even the circular dependency condition

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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