Do I need to make synchronized getter to make class threadsafe?

Question

I've code:

class Counter {
    private int v;

    public synchronized int inc() {
        v = v + 1;
        return v;
    }

    public int get() {
        return v;
    }
}

What do I need minimum (for performance sake) && (please don't use concurrent package as well as other packages other than java.lang I just wanna study java basics for now) to do

make private volatile int v;
make public int synchronized get() {...
nothing (everything is ok as is)

to make the code above thread safe?

The question Should getters and setters be synchronized? doesn't give the answer due to the ambiguity:

It is a common mistake to assume that synchronization needs to be used only when writing to shared variables; this is simply not true.

For each mutable state variable that may be accessed by more than one thread, all accesses to that variable must be performed with the same lock held. In this case, we say that the variable is guarded by that lock.
Normally, you don't have to be so careful with primitives

So I don't figure from there out the answer in case of primitive int

please don't use concurrent package as well as other packages other than java.lang — J.J. Beam, May 14 '19 at 21:53
Why? Things that block other threads from executing code are slow and overkill. I would think atomic s are a perfect solution. — George, May 14 '19 at 21:57
I agree Atomic is the best. But I stuck on **exactly** the problem I described above — J.J. Beam, May 14 '19 at 22:00
@JJBeam You'll also need to make the getter synchronized, because otherwise you could be seeing stale values. — MC Emperor, May 14 '19 at 22:12
@JJBeam But you can't leave the getter without either keyword. — MC Emperor, May 14 '19 at 22:26
volatile cannot be applied to a method. See option **1) make private volatile int v** — J.J. Beam, May 14 '19 at 22:30
For what it's worth: the `java.util.concurrent` package *is* the basics of Java concurrency. The stuff that's in `java.lang` should be considered "for advanced use only". As a beginner, learn `java.util.concurrent` since that's what's designed for day-to-day use. — Daniel Pryden, May 14 '19 at 23:29
@DanielPryden , Great! How about my question? Do I need volatile or can leave the code as is? — J.J. Beam, May 15 '19 at 08:23
You need `synchronized` on the getter to ensure a *happens-before* edge. *volatile* alone is not sufficient here. This is already thoroughly explained in the linked duplicate. — Daniel Pryden, May 15 '19 at 10:16
There is no ambiguity there at all. If you look at the context of the other question, it explains why you don't have to be *so careful*. Note that it is NOT saying that you don't have to be careful at all. It is saying that seeing a stale value for a primitive *may be* less harmful. (I don't agree with that point of view. But what the writer is saying is quite clear, and not self-contradictory.) — Stephen C, May 15 '19 at 10:29
But if we are arguing about *correctness* of the code in a multi-threaded context (i.,e. thread safety) we should not be relying on vague hand-wave arguments. We should look to see what the Java Memory Model says. And is says that you must have a *happens before* relationship to guarantee that the getter will see the result of the `inc` operation. You can do this by declaring the getter as `synchronized` or by declaring the field as volatile. — Stephen C, May 15 '19 at 10:34
As one can see no one gave direct right answer on exactly my question (your latest comment is the first), it's enough evidence the "dublicated" question was at least quite vague — J.J. Beam, May 15 '19 at 11:51
Reopening since the original duplicate target had something about primitives that is confusing everybody, it doesn't make it as clear as it should. A much better duplicate target would be [this one](https://stackoverflow.com/q/3076014/217324) — Nathan Hughes, May 15 '19 at 13:22
The best evidence against marking my question as duplicated is: it's still unclear and no one gave right and short answer even in comments. For now I found the answer by myself (+ Stephen C did the same ) and not in "dublicated" topics but in JMM: synchronized working as happens-before only among monitor acquiring, so if getter is not synchronized it doesn't help without volatile. While thread is inside monitor it doesn't help if non-synchronized method read the value. — J.J. Beam, May 15 '19 at 14:00
This question is actually pretty fascinating. I am not going to ever recommend it in general, but from a technical point of view, I don't see why a `synchronized` setter, `volatile` field and non-synchornized getter wouldn't be thread safe. `volatile` on the field alone won't work because you lose atomicity, but you gain it with the `synchronized` setter. Not having a `synchronized` getter, you lose your happens-before relationship, but you get that back with the `volatile` write and subsequent read. Am I missing something? — John Vint, May 15 '19 at 15:50

score 0 · Answer 1 · answered May 14 '19 at 21:47

0

If you want that class thread safe (and I think that includes avoiding dirty reads), you should choose the first option (make private volatile int v).

The second option (public int synchronized get()) will work too, but synchronization is heavier (-> decreased performance).

Third option (nothing (everything is ok as is)) may produce dirty reads in other threads.

More info: Java volatile keyword

answered May 14 '19 at 21:47

sador

34
4

I agree with you, but are you sure? Let's wait for other comments... I've no Idea who and why downvoted you... – J.J. Beam May 14 '19 at 21:57
I suspect I have been _punished_ for answering instead of marking the question as "duplicate"... – sador May 14 '19 at 22:26
It's not duplicated, due to ambiguity, see the reason at the bottom of my question please – J.J. Beam May 14 '19 at 22:27
Ok, I voted for you and gonna mark your answer as the answer if nobody will explain, don't worry – J.J. Beam May 14 '19 at 22:37

J.J. Beam · Answer 2 · 2019-05-15T15:40:16.047

An unlock on a monitor happens before every subsequent lock on that same monitor.

A write to a volatile field happens before every subsequent read of that same volatile.

So it doesn't make sense "happens before" if the getter thread doesn't use monitor (call synchronized method) because it make sense only in subsequent acquiring monitor.

So either make both getter and setter synchronized OR make the field volatile (and if have only 2 threads: setter and getter don't need setter be synchronized in this case, but if 2 or more setters - need setter be synchronized and volatile as well)

1 setter thread:

class Counter {
    private volatile int v; 

    public int inc() {
        v = v + 1;
        return v;
    }

    public int get() {
        return v;
    }
}

multiple setter threads:

class Counter {
        private volatile int v; 

        public synchronized int inc() {
            v = v + 1;
            return v;
        }

        public int get() {
            return v;
        }
    }

for both case ok, but redundant:

class Counter {
            private int v; // also can be volatile but redundant

            public synchronized int inc() {
                v = v + 1;
                return v;
            }

            public synchronized int get() {
                return v;
            }
        }

Making the field volatile here is not threadsafe, because the increment in the inc method is not atomic and can be interfered with by other threads. You could use one of the java.util.concurrent.atomic classes. if all code using the field is synchronized then volatile is redundant. — Nathan Hughes, May 15 '19 at 14:45
that's why I said: volatile enough only in case of 1 setter and 1 getter (in this case there is no setting race condition). But in case of >1 setters and 1 getter - setter need to be synchronized AND (either getter need to be synchronized OR volatile) — J.J. Beam, May 15 '19 at 14:53

Do I need to make synchronized getter to make class threadsafe?

2 Answers2