views:

98

answers:

6

In the class below, I am using a singleThreadScheduledExecutor. My question is, do I need to synchronize around the access to dummyInt and dummyBoolean?

import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;

public class Playground {

    /**
     * @param args
     */
    public static void main(String[] args) {
        startThread();

    }

    private static void startThread() {
        ScheduledExecutorService timer = Executors
                .newSingleThreadScheduledExecutor();
        Runnable r = new Runnable() {
            int dummyInt = 0;
            boolean dummyBoolean = false;

            @Override
            public void run() {
                dummyInt = dummyInt + 1;

                if (dummyBoolean) {
                    dummyBoolean= false;
                } else {
                    dummyBoolean= true;
                }

            }

        };

        timer.scheduleAtFixedRate(r, 0, 100, TimeUnit.MILLISECONDS);

    }

}
A: 
dummyInt = dummyInt + 1;

This statement is actually 3 separate operations:

  1. Read the value of dummyInt
  2. Add 1
  3. Write the value back to dummyInt

So yes you do need to synchronize this. It is possible for one thread to read the value, then another thread does all three operations, and when the first thread finishes the value is only increased by 1 (I hope this makes sense :P).dummyBoolean is similar. You read it in the if statement and the write the new value.

EDIT

Sorry for not reading the question correctly. According to the javadoc this shouldn't need sync.

pablochan
I get that it is 3 separate operations.. The reason I even asked this was because I am using a "singleThreadedExecutor" so I thought maybe synch wouldnt be required.
Grasper
No, every thread will get its own instance of that Runnable subclass.
Pointy
+3  A: 

No, you don't. There is only a single thread accessing the values, so no synchronization is required.

Thomas Lötzer
+2  A: 

Every thread you start with that mechanism will have its own instance of the "Runnable" subclass you define and instantiate. Therefore, there can't possibly be contention.

Pointy
A: 

No, but it probably wouldn't hurt. Since you're using a newSingleThreadScheduledExecutor which promises that

Tasks are guaranteed to execute sequentially, and no more than one task will be active at any given time.

but if you ever change executors, let the Runnable get out so other can invoke it, or check the value externally, then you'll wish you'd synchronized it.

Ry4an
A: 

You dont have to make it Synchronized

harigm
+1  A: 

Do you need to? No. Only a single thread will ever access the variables in the current implementation, so it is thread safe.

Would it hurt performance to? Well, yes, but not by as much as you would probably expect. The modern JIT compilers are quite happy to spot that the synchronization is unnecessary in the current usage, and eliminate virtually all the overhead from the compiled code - but there would be a little overhead remaining that checked whether the assumption of single-threaded access was still valid. And of course, there is the overhead of JITting this.

Would it ever hurt not to synchronize? Well, possibly, if the implementation ever changed and the assumption of single thread access no longer held - and the developer making the change overlooked that consequence of their change.

But actually, in this context, is that likely to occur? Maybe not - all the code is contained in a very small area...

I'd probably leave some kind of comment documenting the assumption. I might even annotate the class as @NotThreadSafe, if I was using the JCIP annotations anywhere else in my project. A discussion of the use of those annotations can be found in the Java Concurrency In Practice book by Brian Goetz, and the annotation source code and a jar file are available for download from the book's web site.

Bill Michell