On 17/02/11 20:25, Jeffrey Ollie wrote:
I think that I've tracked down an NPE in the crosby_integration branch of the splitter. The SplitProcessor is using a BlockingQueue to queue up elements for writing. The poll() method can potentially return a null if the queue is empty. My question is, should the poll() method call be switched to a take() method call - the take() method will block if the queue is empty, or should I guard the usage of elements for null like in the patch below?
I think that if you get a null then you should break out of the loop so that the thread can pick up the next work package. By waiting or spinning while the workPackage is empty, the thread wastes time (and I don't suppose there is any guarantee that anything will ever be added). But lets look at what is happening, this is my take: The synchronized line does nothing useful so it can be removed. Then there is a check that the queue is not empty. Later we try to read from it and find that it is empty. This means that some other thread is also reading from the same queue. From the code this is possible, but possibly unintended - I would expect that the reason that the work is parcelled up in the first place is to avoid excessive contention reading work items by different threads from a single queue. In any case, the check for empty is also useless and so can be removed. The remaining code would then look like this: ArrayList<Element> elements; while ((elements = workPackage.inputQueue.poll()) != null) { try { for (Element element : elements ) { processElement(element, workPackage.writer); } } catch (IOException e) { throw new RuntimeException("Thread " + Thread.currentThread().getName() + " failed to write element ", e); } } Also attached as a patch, could you try it please. Oh and thanks for looking into these problems, I'm going to look at your second patch now. Best wishes ..Steve