0

We are getting this exception which seems impossible given our code... We add items to a static linked list and we start a thread to process items added on those lists. If the static thread is dead or null when adding items to the list we start it off again. The thread makes itself null once all items are processed.

See small snippet below

Code:

public class LiveKPIUpdates extends Thread {

  private static final int MAX_QUEUE_SIZE = 1000;
  private static LinkedList < UpdateLine > highQueue = new LinkedList < UpdateLine > ();
  private static LinkedList < UpdateLine > lowQueue = new LinkedList < UpdateLine > ();
  private static UpdateThread updateThread = null;

  /**
   * Thread to process live update queues.
   * High priority items are processed first.
   */
  static class UpdateThread extends Thread {
    @Override
    public void run() {
      while (!highQueue.isEmpty() || !lowQueue.isEmpty()) {
        UpdateLine update = !highQueue.isEmpty() ? highQueue.removeFirst() : lowQueue.removeFirst(); // <<<<< !!! EXCEPTION HAPPENS HERE... Why/How? !!! >>>>>>>>>>>>>>>
        //Do stuff with update        
      }
      updateThread = null;
    }
  }

  /**
   * Adds the live update to a queue to be processed.
   * If the queue is full, the update may be ignored.
   *
   * @param appender The machine who sent these lines
   * @param lines The lines the machine sent. (Can be tray file or machine log lines)
   */
  synchronized public static void updateKPIWithMachineLines(AppenderID appender,
    String fileName, Date fileDate, LinkedList < String > lines) throws ParseException, SQLException {
    for (String line: lines) {
      if (line.startsWith("something...")) {
        if (highQueue.size() < MAX_QUEUE_SIZE) {
          highQueue.add(new UpdateLine(appender, fileName, fileDate, line));
        } else {
          TLSystemAction.log("Live updates high priority queue full", appender);
        }
      } else {
        if (lowQueue.size() < MAX_QUEUE_SIZE) {
          lowQueue.add(new UpdateLine(appender, fileName, fileDate, line));
        } else {
          TLSystemAction.log("Live updates low priority queue full", appender);
        }
      }
    }

    if (updateThread == null) {
      updateThread = new UpdateThread();
      updateThread.start();
    } else if (!updateThread.isAlive()) {
      updateThread.interrupt();
      updateThread = new UpdateThread();
      updateThread.start();
    }
  }

Exception:

[#|2015-03-12T11:12:10.614+1300|SEVERE|glassfish3.0.1|javax.enterprise.system.std.com.sun.enterprise.v3.services.impl|_ThreadID=617736;_ThreadName=Thread-1;|java.util.NoSuchElementException
at java.util.LinkedList.remove(LinkedList.java:788)
at java.util.LinkedList.removeFirst(LinkedList.java:134)
at api.tl.put.LiveKPIUpdates$UpdateThread.run(LiveKPIUpdates.java:67)

EDIT: I am more interested in how it is possible for it to get to the lowQueue.removeFirst() if lowQueue is empty. The logic seems to protect from that.

Riaan Schutte
  • 535
  • 1
  • 5
  • 14
  • 1
    The most likely cause is that your synchronization is incorrect. UpdateThread does not synchronize, so it accesses mutable objects (lists) without necessarily having visibility over their current state. In the absence of correct concurrency controls, you have no guarantees about what's going to happen. – Tim Mar 12 '15 at 03:06
  • Thanks @Tim , Yes this seems to be what is happening. – Riaan Schutte Mar 13 '15 at 02:50

2 Answers2

1

Looks like it is not possible to get this kind of exception provided the list empty conditions you have in the code. It may also look like @muasif80's answer will fix it. However, it is possible to see unexpected behavior in your code because of race conditions. You have seen only one kind of problem; you may see more such unexpected and unexplained failures in real life usage. java.util.LinkedList is not thread safe class. You will see such unexpected behavior when you share the instance of a thread unsafe class between multiple threads. Synchronize the list access and you should see the code running without problems. Since you have more than one list you should synchronize on a common object.

public class LiveKPIUpdates extends Thread {
  private static final int MAX_QUEUE_SIZE = 1000;
  private static LinkedList < UpdateLine > highQueue = new LinkedList < UpdateLine > ();
  private static LinkedList < UpdateLine > lowQueue = new LinkedList < UpdateLine > ();
  private static UpdateThread updateThread = null;
  private static Object lock = new Object();

  /**
   * Thread to process live update queues.
   * High priority items are processed first.
   */
  static class UpdateThread extends Thread {
    @Override
    public void run() {
      boolean queueHasData = false;
      synchronized(lock) {
        queueHasData = !highQueue.isEmpty() || !lowQueue.isEmpty();
      }
      while (queueHasData) {
        UpdateLine update;
        synchronized(lock) {
          update = !highQueue.isEmpty() ? highQueue.removeFirst() : lowQueue.removeFirst();
          queueHasData = !highQueue.isEmpty() || !lowQueue.isEmpty();
        }
        //Do stuff with update        
      }
      updateThread = null;
    }
  }

  /**
   * Adds the live update to a queue to be processed.
   * If the queue is full, the update may be ignored.
   *
   * @param appender The machine who sent these lines
   * @param lines The lines the machine sent. (Can be tray file or machine log lines)
   */
  synchronized public static void updateKPIWithMachineLines(AppenderID appender,
    String fileName, Date fileDate, LinkedList < String > lines) throws ParseException, SQLException {
    for (String line: lines) {
      if (line.startsWith("something...")) {
        synchronized(lock) {
          if (highQueue.size() < MAX_QUEUE_SIZE) {
            highQueue.add(new UpdateLine(appender, fileName, fileDate, line));
          } else {
            TLSystemAction.log("Live updates high priority queue full", appender);
          }
        }
      } else {
        synchronized(lock) {
          if (lowQueue.size() < MAX_QUEUE_SIZE) {
            lowQueue.add(new UpdateLine(appender, fileName, fileDate, line));
          } else {
            TLSystemAction.log("Live updates low priority queue full", appender);
          }
        }
      }
    }

    if (updateThread == null) {
      updateThread = new UpdateThread();
      updateThread.start();
    } else if (!updateThread.isAlive()) {
      updateThread.interrupt();
      updateThread = new UpdateThread();
      updateThread.start();
    }
  }

Though this code may work as expected after synchronized access to the lists, it is not the best solution. This is a classical producer-consumer problem. Java has wait...notify mechanism for handling producer consumer problems. You get many examples of producer consumer on web. Read, learn and implement wait...notify pattern.

RaviH
  • 3,544
  • 2
  • 15
  • 14
0

Change this

while (!highQueue.isEmpty() || !lowQueue.isEmpty()) {
    UpdateLine update = !highQueue.isEmpty() ? highQueue.removeFirst() : lowQueue.removeFirst(); // <<<<< !!! EXCEPTION HAPPENS HERE... Why/How? !!! >>>>>>>>>>>>>>>
    //Do stuff with update        
  }

to this

while (!highQueue.isEmpty() || !lowQueue.isEmpty()) {
    UpdateLine update = !highQueue.isEmpty() ? highQueue.removeFirst() : (!lowQueue.isEmpty() : lowQueue.removeFirst() ? [something else that you know]); // <<<<< !!! EXCEPTION HAPPENS HERE... Why/How? !!! >>>>>>>>>>>>>>>
    //Do stuff with update        
  }

Basically your lowQueue can be empty according to your code where it calls lowQueue.removeFirst() because the || operator in the while() passes if the first condition is true and it does not check the second condition in that case because it is an or operation.

You can check this

Does Java evaluate remaining conditions after boolean result is known?

or this

http://en.wikipedia.org/wiki/Short-circuit_evaluation

Community
  • 1
  • 1
muasif80
  • 5,586
  • 4
  • 32
  • 45
  • You are correct in that it will prevent the exception but short circuiting conditions is not the issue here. I cannot understand why it is even going to the else case. If lowQueue is empty then highQueue is not and the inline condition checks highQueue first so I cannot understand why we are getting the exception and how we are getting to the lowQueue.removeFirst() if it is empty. – Riaan Schutte Mar 12 '15 at 00:54
  • 1
    "Basically your lowQueue can be empty according to your code where it calls lowQueue.removeFirst()" is not true. The logic prevents from that. – Riaan Schutte Mar 12 '15 at 02:44
  • More than the logic it is the observation about the code what is it doing. Whatever logic is there in other parts of program but this particular piece is not preventing the call in case lowqueue is empty. That was what i mentioned. – muasif80 Mar 12 '15 at 03:22