Spurious wakeups in Java’s BlockingQueue.take()

Posted on

Problem

I wrote this code where I trap InterruptedException‘s in a blocking queue and try again to consume from the same queue thinking of spurious wakeups. I don’t know if in this case I should be thinking of spurious wakeups since I am not calling wait() directly.

Basically, the worry that someone raised is that if the take throws an InterruptedException, I should stop processing (so the while should be inside the try, the opposite of what I did). There is already a mechanism to stop processing by calling a public method in the class, but I don’t know if processing should also be stopped by an InterruptedException.

Does anyone have any thoughts?

private BlockingQueue<MyMessage> queue;
...
@Override
public void run() {
   log.debug("[run] Starting processing");

   while (runningController.isRunning()) {
      try {
         MyMessage message = queue.take(); // will block if empty

         assert message != null; // a blocking queue can never return null

         RuntimeMessage parsedMsg = processMessage(message);

         if (parsedMsg != null) {
            outputConsumer.messageArrived(parsedMsg);
         } else {
            log.warn("[run] Unparsed message 0x" + Integer.toHexString(message.getCommandType()));
         }

      } catch (InterruptedException exc) {
         log.error("[run] Interrupted while reading from queue", exc);
      } catch (MessageConfigurationException exc) {
         log.error("[run] Error finding structure configuration for received message", exc);
      }
   }
}

Solution

When you write, that message cannot be null, why are you asserting for it then?
That code is IMO noise. I’d rather remove it. Other than that, the comment on it is even more noisy. Your assert says exactly what your comment says. Why keep it?

Same should go for the //will block if empty. It’s noise, as it adds no value to the understanding of the code. It’s clear that BlockingQueue.take() will block when there’s no messages, why would there be a need to write a comment on that?

Currently you catch inside your while loop. this means your execution continues, even when InterruptedException or MessageConfigurationException is thrown. You should keep that, given the case it is desired behavior. You have to decide on a case to case basis. We miss the context to decide on that, that’s why its your job, isn’t it 😉

If you want to stop execution when you caught an exception, just call that public method you mentioned. It’s not like calls from other public members are forbidden 😉

Oh and a minor nitpick, the name for your BlockingQueue could be better, I’d probably go for messages.

Leave a Reply

Your email address will not be published. Required fields are marked *