Is it safe to put a stateless callback into BlockingQueue?

Question

I've got the following pretty straightforward callback interface and a POJO class:

public interface Action{
    public void doAction();
}

public class Person{
     private String name;
     private String address;
     //...etc
     //GET, SET, toString
}

And I'm going to use it as follows:

public class ActionExecutor{

    private static final Logger logger = LogManager.getLogger(ActionExecutor.class);
    private final BlockingQueue<Action> blockingQueue = new LinkedBlockingQueue(2000);

    public void execute(final Person p){
        //modify state of p in some way
        blockingQueue.put(new Action(){
            public void doAction(){
                logger.info("Execution started: " +p.toString );
                //do some other job
        });
    }
}

BlockingQueue here is used to implement producer-consumer.

Question: Is it guaranteed that a consumer thread taking action from the BlockingQueue will write a correct log message? I.e. it observes a correct state of Person? But I'm not strictly sure about that.

I think no, it's not guaranteed, as there's no happens before order between modifications made bya producer and reading by a producer.

No its not guaranteed. The state of Person p could change under you before the doAction method is called. You could get the string of p.toString() and pass that string into your Action constructor and store it as a member of the Action instance. — bhspencer, Apr 06 '16 at 14:27
@bhspencer So we can only publish safely thread safe objects? — St.Antario, Apr 06 '16 at 14:57
I am not sure what you mean by publish here. I don't think the issue is about thread safety but about state. When you put your Action on the queue the Person it has a reference to is not in a locked state, so when your action finally runs the state of the person could be different than when you created the Action. Person p may very well be thread safe but it if it is mutable its state can change under you. — bhspencer, Apr 06 '16 at 15:39
Maybe you should write a method that makes a clone of your Person and passes that cloned instance to your Action on construction. — bhspencer, Apr 06 '16 at 15:41
You cannot know the state of an object in the queue if multiple threads might change it. The "blocking" functionality of the blocking queue only applies to loading the reference into the queue. — DwB, Apr 06 '16 at 15:56
@DwB Actually that's what I was concerned about. It appeared to me a little blurred since we operate on local variable... — St.Antario, Apr 06 '16 at 17:54
The thing creating the status message must prevent the state from changing while it is creating the status message. The "store in a local variable" is not as important as preventing the state from changing while creating the status message. It is fine to have the state change after the message is created, just synchronize access to the list that stores the messages. also you probably don't need to write the log message in a synchronized block, just control access to the message list. — DwB, Apr 06 '16 at 18:08

score 3 · Accepted Answer · answered Apr 06 '16 at 15:48

The answer is that it depends.

If you never modify the Person instance after that, then it is guaranteed. There is a happens-before relationship, as stated in the docs:

Memory consistency effects: As with other concurrent collections, actions in a thread prior to placing an object into a BlockingQueue happen-before actions subsequent to the access or removal of that element from the BlockingQueue in another thread.

However, I still wouldn't do that. If something modified the Person instance after the callback has been placed on the queue, then there are no guarantees. You could say that the logger is guaranteed to print the state that is at least as up-to-date as it was when it was added to the queue.

So the consumer will see these modifications:

public void execute(final Person p){
    //modify state of p in some way
    blockingQueue.put(new Action(){

But not those made after that. And since you keep p around after passing it to the new Action() object, I'd avoid this. Instead, I'd do something like this:

public void execute(final Person p){
    //modify state of p in some way
    blockingQueue.put(new Action() {
        final String executionName = p.toString();
        public void doAction(){
            logger.info("Execution started: " + executionName);
            //do some other job
    });
}

Now the state is captured in a final variable. The consumer will see that value, no matter what the state of p is at that moment.

Instead of performing the toString in the anonymous Action class, I believe that you should implement a method on p that tracks the state of p by implementing a method on the Person class that adds strings to a list (synchronize the access with other changes to person state to guarantee one message per state change) and providing access to the state-change-list with a public method, perhaps getStateChangeMessage(). — DwB, Apr 06 '16 at 16:01
@DwB, I'm not sure that the Person class is the right place to do it. Not enough information in the question to figure that out. If it's a simple POJO, then it's probably not the right place. But that's beyond the scope of this question, I think. — Sergei Tachenov, Apr 06 '16 at 16:05

Is it safe to put a stateless callback into BlockingQueue?

1 Answers1