4

In the Java Concurrency in Practice book you can find following code:

@ThreadSafe
public class SafePoint { 
    @GuardedBy("this") private int x, y;
    private SafePoint(int[] a) { this(a[0], a[1]); }
    public SafePoint(SafePoint p) { this(p.get()); }
    public SafePoint(int x, int y) { 
        this.x = x;
        this.y = y;
    }
    public synchronized int[] get() { return new int[] { x, y };
    }
    public synchronized void set(int x, int y) { this.x = x;
        this.y = y;
    }
}

This marked as @ThreadSafe.

I am pretty sure that this class is not thread-safe (if I understand this term correctly).

Example:

 SafePoint racePublishedSafePoint; // field

 // thread 1:
 racePublishedSafePoint = new SafePoint(1,1);

 // thread 2:
 SafePoint sp;
 while(true){
   SafePoint sp = racePublishedSafePoint;
   if(sp != null) break;
 }
 System.out.println(sp.get()[0]);
 System.out.println(sp.get()[1]);

I believe that there is several possible outcomes:

  1. The application doesn't finish
    else
  2. If application finished, we can see
    a) 0 0
    b) 0 1
    c) 1 0
    d) 1 1

Am I right?

If true, why did the author mark the class as thread safe? I thought that thread-safe class - class which can be used in concurrent application without sophisticated analysis.

What did author want to say?

P.S.

I have read the Private constructor to avoid race condition

...and my topic is not duplicate.

Cœur
  • 37,241
  • 25
  • 195
  • 267
gstackoverflow
  • 36,709
  • 117
  • 359
  • 710
  • "Am I right?" No. You're wrong in all the claims you make in your post, and you seem to claim that you know better than the writers of "Java Concurrency in Practice" when you say "I am fully sure that this class is not thread-safe". – Kayaman Feb 21 '17 at 13:25
  • @Kayaman I fully understand that book authors know more than me, but they interpret his thougts to text and then I try to understand this text. This chain sometimes distort the orifinal thought. wheteher I incorrect understand thread-safe whether something else. And why do you remove your post? – gstackoverflow Feb 21 '17 at 13:30
  • Let's get back to the basics: `SafePoint` **is** thread-safe. *Your example code* is not thread-safe. – Kayaman Feb 21 '17 at 13:39
  • @Kayaman lets repeat one more time. Please provide difference between java.util.Date and SafePoint. What exact means thread-safety? – gstackoverflow Feb 21 '17 at 13:40
  • You can use an instance of `SafePoint` from multiple threads safely, whereas using 'Date` concurrently will result in the corruption of the internal state of the object. YOUR BROKEN EXAMPLE CODE DOES NOT CORRUPT THE INTERNAL STATE OF `SafePoint`. IT DOES NOT SAFELY PUBLISH THE INSTANCE, SO IT'S BROKEN CODE BUT IT DOES NOT BREAK `SafePoint`. – Kayaman Feb 21 '17 at 13:42
  • @Kayaman I can wrap Date with synchronized and it will work correct. I just provided race publication. For example immutable objects can be publicated in any way – gstackoverflow Feb 21 '17 at 13:44
  • That still doesn't make `Date` a thread-safe class. It just means that you're using external code to (try to) use it in a thread-safe fashion. – Kayaman Feb 21 '17 at 13:45
  • @Kayaman thread safety classes should be publicated safety? immutable - more safety than thread-safe? – gstackoverflow Feb 21 '17 at 13:46
  • @Kayaman I know that you disagree but in provided example you could see corrupted internal state from another thread – gstackoverflow Feb 21 '17 at 13:48
  • No, the internal state isn't corrupted. Your code is just broken so it displays broken values. I am getting tired of arguing with you. You don't understand things, but you still insist on saying that black is white. – Kayaman Feb 21 '17 at 13:48
  • @Kayaman state is not corrupted but we want to provide visibility atomically, but at this case - it is ptoblem – gstackoverflow Feb 21 '17 at 14:06
  • Yes it's a problem. You caused that problem by writing broken code. Don't do that. Also don't blame `SafePoint`. It's you who wrote the broken code, nobody else. If you write broken code, don't ask "why does this code not work correctly", since you obviously know that the code is broken (based on you naming the variable `racePublishedSafePoint`). – Kayaman Feb 21 '17 at 14:15
  • @Kayaman but if replace SafePoint with String - code will correct – gstackoverflow Feb 21 '17 at 14:21
  • Yeah well it's not very useful to compare `SafePoint` and `String`. – Kayaman Feb 21 '17 at 14:27
  • @Kayaman Why? String Thread-safety? – gstackoverflow Feb 21 '17 at 14:29
  • They're entirely different classes. – Kayaman Feb 21 '17 at 14:33
  • @Kayaman but my bad code works good for this class – gstackoverflow Feb 21 '17 at 15:01
  • Do you think that means your code isn't bad? If it works with `String`, but doesn't work with `SafePoint`. Are you still blaming the `SafePoint` class? – Kayaman Feb 21 '17 at 15:02
  • @Kayaman I mereley don't understand criterias if good or bad code. – gstackoverflow Feb 21 '17 at 15:45

1 Answers1

6

I agree with the OP that the example seems to violate the usual understanding of @ThreadSafe guarantees. The unsafe publication races are very real, and of course you can see puzzling behaviors when publishing SafePoint via the race. Some real-life thread-safe classes survive racy publication (String being a notorious example of this), adding up to the confusion.

As far as JCIP narrative goes, I have no paper or electronic copy handy, so reached out to Doug Lea (one of the primary authors), who said:

I think the part of confusion is in the JCIP text. I don't think @ThreadSafe covers publication races, or any other races that could be encountered during construction. Publication safety is treated separately.

Still, I can see how people could think otherwise. It is one of the reasons for us exploring always placing release fences in constructors.

The last part Doug is talking about is briefly described in "All Fields Are Final", including the motivation, experimental patches, and performance estimates.

Aleksey Shipilev
  • 18,599
  • 2
  • 67
  • 86