Null pointer exception in server

Question

This is a client-server programm. For each client server has a method, which checks if there are some messages to this client.

Code:

        while (bool) {
            for(int j = 0;j<Start.bases.size();j++){
                if(Start.bases.get(j).getId() == id){
                    if(!Start.bases.get(j).ifEmpty()){
                        String output = Start.bases.get(j).getMessage();
                        os.println(output);
                        System.out.println(output +" *FOT* "+ addr.getHostName());
                    }
                }

            }

Each thread has an id. So everything seems to be OK, but I get strange null pointer Exception at this line

if(Start.bases.get(j).getId() == id){

id - integer. It is really strange, because I have run in debug this part and checked that "bases" and "id" are not null and bases have apropriate fields. bases is not empty.

By the way bases is static(because every thread can use it) and bases is declared before this method is used.

This line doesn't cause problems

            for(int j = 0;j<Start.bases.size();j++){

May it is because of method getId() ?

public int getId(){
    return id;

}

What is the problem?

Edited.

  static ArrayList<Base> bases;
  bases = new ArrayList<Base>();

Class Base:

 public class Base {
private ServerThread st;
private int id;
private String name;
private ArrayList<String> messages;

public Base(String n, ServerThread s_t, int i_d){
    messages = new ArrayList<String>();
    st = s_t;
    name = n;
    id = i_d;
}

public String getName(){
    return name;
}
public int getId(){
    return id;
}
public ServerThread getThr(){
    return st;
}
public String getMessage(){
    String ret = "";

    if(!messages.isEmpty()){
        ret = messages.get(0);
        messages.remove(messages.get(0));
    }

    return ret;
}

public void addMessage(String m){
    messages.add(m);
}

public boolean ifEmpty(){
    return messages.isEmpty();
}
  }

Thanks.

Answer 1

Given this:

"I have run in debug this part and checked that "bases" and "id" are not null and bases have apropriate fields"

and this:

bases is static(because every thread can use it)

I think it's pretty likely that you have a race condition. In a race condition, there are two threads simultaneously accessing the same data structure (in this case, Start.bases). Most of the time, one thread's code completes faster, and everything goes the way you expect them to, but occasionally the other thread gets a head-start or goes a little faster than usual and things go "boom".

When you introduce a debugger with a break point, you pretty much guarantee that the code with the break point will execute last, because you've stopped it mid-execution while all your other threads are still going.

I'd suggest that the size of your list is probably changing as you execute. When a user leaves, is their entry removed from the "base" list? Is there some other circumstance where the list can be changed from another thread during execution?

The first thing I'll suggest is that you switch your code to use iterators rather than straight "for" loops. It won't make the problem go away (it might actually make it more visible), but it will make what's happening a lot clearer. You'll get a ConcurrentModificationException at the point where the modification happens, rather than the less helpful NullPointerException only when a certain combination of changes happens.):

        for(Base currentBase : Start.bases)
        {
            if(currentBase.getId() == id && !currentBase.ifEmpty())
            {
                String output = currentBase.getMessage();
                os.println(output);
                System.out.println(output +" *FOT* "+ addr.getHostName());
            }
        }

If you do get a concurrent modification exception with the above code, then you're definitely dealing with a race condition. That means that you'll have to synchronize your code.

There are a couple of ways to do this, depending on how your application is structured.

Assuming that the race is only between this bit of code and one other (the part doing the removing-from-the-list), you can probably solve this scenario by wrapping both chunks of code in

synchronized(Start.bases)
{
   [your for-loop/item removal code goes here]
}

This will acquire a lock on the list itself, so that those two pieces of code will not attempt to update the same list at the same time in different threads. (Note that it won't stop concurrent modification to the Base objects themselves, but I doubt that's the problem in this case).

All of that said, any time you have a variable which is read/write accessed by multiple threads it really should be synchronized. That's a fairly complicated job. It's better to keep the synchronization inside the object you're managing if you can. That way you can see all the synchronization code in one place, making you less likely to accidentally create deadlocks. (In your code above, you'd need to make the "for" loop a method inside your Start class, along with anything else which uses that list, then make "bases" private so that the rest of the application must use those methods).

Without seeing all the other places in your code where this list is accessed, I can't say exactly what changes you should make, but hopefully that's enough to get you started. Remember that multi-threading in Java requires a very delicate hand!

Answer 2

In this line of code: (Start.bases.get(j).getId() == id

you may have such exception in such cases:

1) bases is null - you said its wrong
2) bases.get(j) - it may occur only if you collection size was reduced during iteration(as mentioned Gray)
3) Start.bases.get(j).getId() is null. But as you mentioned getId() method return primitive int, so its not the case as in this situation you receive null ponter while casting - in line "    return id;".

So you should check second case.