-1

I'm reading Thinking in JAVA (Ed4, by Bruce Eckel), which says:

Note that it’s especially important to make fields private when working with concurrency; otherwise the synchronized keyword cannot prevent another task from accessing a field directly, and thus producing collisions.

I am confused and finally get this demo:

public class SimpleSerial {

    public static void main(String[] args) throws IOException {
        ShareObject so = new ShareObject();
        Thread thread1 = new Thread(new ThreadOperation(so, "add"));
        Thread thread2 = new Thread(new ThreadOperation(so, "sub"));
        thread1.setDaemon(true);
        thread2.setDaemon(true);
        thread1.start();
        thread2.start();
        System.out.println("Press Enter to stop");
        System.in.read();
        System.out.println("Now, a=" + so.a + " b=" + so.b);
    }
}

class ThreadOperation implements Runnable {
    private String operation;
    private ShareObject so;

    public ThreadOperation(ShareObject so, String oper) {
        this.operation = oper;
        this.so = so;
    }

    public void run() {
        while (true) {
            if (operation.equals("add")) {
                so.add();
            } else {
                so.sub();
            }
        }
    }
}

class ShareObject {
    int a = 100;
    int b = 100;
    public synchronized void add() {
        ++a;
        ++b;
    }
    public synchronized void sub() {
        --a;
        --b;
    }
}

Every time the values of a and b are different. So why?

The demo also mentioned if the thread sleep() for short time, i.e., re-write the run() method in ThreadOperation:

public void run() {
    while (true) {
        if (operation.equals("add")) {
            so.add();
        } else {
            so.sub();
        }
        try {
            TimeUnit.MILLISECONDS.sleep(1);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }
}

then values of a and b are the same. So again, Why? What happens behind sleep()?

xin
  • 309
  • 2
  • 19
  • 1
    Which actually has nothing to do with the title of your question. If fields are not private, then any class (and therefore any thread) can access them without synchronization. For a statement like `synchronized(so)` to have any meaning, you have to enforce that requirement for all code in your system. Public fields don't. – markspace Mar 07 '18 at 02:55
  • `so.a` and `so.b` are accessed without any synchronization on `System.out.println("Now, a=" + so.a + " b=" + so.b);` because of that Java Memory Model doesn't guarantee that your main thread will see actual values of `so.a` and `so.b` at all. – Ivan Mar 07 '18 at 02:58
  • @ElliottFrisch First, I just tried that `synchronized(so)` in `while` loop did not imporve the behavior, maybe I missed something else? Second, in my understanding, if `thread1` goes into `add()` then it can also goes into `sub()` and `thread2` can go into **neither** of the two methods, that's the way object lock works , right? – xin Mar 07 '18 at 02:59
  • Access modifiers such as `public`, `private`, `protected`, have nothing to do with synchronization. For example, you can have an immutable object with all fields `public` and have it used by various threads, at the end of the day, the object will remain unchanged (provided reflection does not play a role). – smac89 Mar 07 '18 at 03:42
  • @EJP. I understand. But in this example, i think the filed is accessed directly only by main task, after `thread1` and `thread2` finish. so why does this matter? – xin Mar 07 '18 at 03:53

1 Answers1

0

With sleep() it becomes probable that the println() executes while the threads are sleeping. The program is still very not thread-safe.

You could fix it by adding a synchronized print() method to SharedObject eg:

public synchronized void print() {
    System.out.println("Now, a=" + a + " b=" + b);
}

and calling that on the last line of main instead of the current unsynchronized accesses.

metaquanta
  • 11
  • 2