patternjavaMinor
Spurious wakeups in Java's BlockingQueue.take()
Viewed 0 times
blockingqueuewakeupstakejavaspurious
Problem
I wrote this code where I trap
Basically, the worry that someone raised is that if the
Does anyone have any thoughts?
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 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
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
Currently you catch inside your while loop. this means your execution continues, even when
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
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.Context
StackExchange Code Review Q#48488, answer score: 3
Revisions (0)
No revisions yet.