Thread safety implications of Spring DI

Question

In my contrived example, what are the implications for thread safety with regard to the list of teamMembers?

Can I rely on the state of the list as seen by the run() method to be consistent?

Assuming

1. the setATeamMembers method is called only once, by spring when it is creating the ATeamEpisode bean

2. the init method is called by spring (init-method) after #1

3. the ATeamMember class is immutable

   • Do I need to declare the teamMembers volatile or similar?

   • Are there any other hideous problems with this approach that I'm overlooking?

Apologies if this is obvious, or a clear failure to rtfm

Thanks and regards

Ed

package aTeam;

import java.util.ArrayList;
import java.util.List;
import java.util.Random;

public class ATeamEpisode implements Runnable{

    private List<ATeamMember> teamMembers;

    /* DI by spring */
    public void setATeamMembers(List<ATeamMember> teamMembers){
        this.teamMembers = new ArrayList<ATeamMember>(teamMembers);    
    }

    private Thread skirmishThread;

    public synchronized void init(){
        System.out.println("Starting skirmish");
        destroy();
        (skirmishThread = new Thread(this,"SkirmishThread")).start();
    }
    public synchronized void destroy(){
        if (skirmishThread != null){
            skirmishThread.interrupt();
            skirmishThread=null;
        }
    }

    private void firesWildlyIntoTheAir(ATeamMember teamMember){
        System.out.println(teamMember.getName()+" sprays the sky..");
    }

    @Override
    public void run() {
        try {
            Random rnd = new Random();
            while(! Thread.interrupted()){
                firesWildlyIntoTheAir(teamMembers.get(rnd.nextInt(teamMembers.size())));
                Thread.sleep(1000 * rnd.nextInt(5));
            }
        } catch (InterruptedException e) {
            System.out.println("End of skirmish");
            /* edit as per Adam's suggestion */
           // Thread.currentThread().interrupt();
        }
    }
}

Answer 1

If, as you say, setATeamMembers is called only once, and no other part of your code either replaces this collection, then there is no point in making it volatile. Volatile indicates that a member can be written by different threads.

Considering no part of your code seems to be updating this collection either, you might want to consider making the collection explicitly unmodifiable, for instance by using Collections.unmodifiableList(). This makes it clear to you, and others, that this collection won't be modified, and will throw a big fat exception in your face if you try to modify it regardless.

Spring's lazy initialization is, AFAIR, thread safe.

Answer 2

Maybe. The List interface as such isn't thread safe and no matter what you do, it can't be made thread safe on the consumer side.

What you need to do is create a thread safe list (the Java runtime has a couple of implementations) and use one of those for the teamMembers bean.

Accessing the bean via the field teamMembers isn't an issue because the other threads don't create a new instance, they change the state (ie. the data inside of) the teamMembers bean.

So the bean must make sure that changes to its internal structure are correctly synchronized.

In your case, you will need a special list implementation which returns a random element from the list. Why? Because the value for teamMembers.size() can have changed when teamMembers.get() is called.

A simple way to achieve this is to wrap all method calls in this code:

 synchronized(teamMembers) { ... }

But you must be sure that you really caught all of them. The most simple way to achieve that is, as I said above, to write your own list which offers all the special methods that you need. That way, you can use locks or synchronized inside of the methods as necessary.