4

Is it correct that the items that I added in my constructor to my volatile linked list below may not be visible to other threads

class ProductPrice {
  private volatile LinkedList<QuantityPrice> quantityPriceList;
  public ProductPrice(List<QuantityPrice> quantityPriceListParam) {
      this.quantityPriceList = new LinkedList<QuantityPrice>();
      quantityPriceList.addAll(quantityPriceListParam);
  }
}

Would the following code where i assign the volatile variable after the list is loaded fix the issue. Because all happen before operations would also be visible.

private volatile LinkedList<QuantityPrice> quantityPriceList;
public ProductPrice(List<QuantityPrice> quantityPriceListParam) {
    LinkedList<QuantityPrice> tempQuantityLinkedList = new LinkedList<QuantityPrice>();
    tempQuantityLinkedList.addAll(quantityPriceListParam);
    this.quantityPriceList = tempQuantityLinkedList;
}

and in this case could i just make the variable final and get the same effect of having all items visible to other threads.

private final LinkedList<QuantityPrice> quantityPriceList;
public ProductPrice(List<QuantityPrice> quantityPriceListParam) {
    LinkedList<QuantityPrice> tempQuantityLinkedList = new LinkedList<QuantityPrice>();
    tempQuantityLinkedList.addAll(quantityPriceListParam);
    this.quantityPriceList = tempQuantityLinkedList;
}
richs
  • 4,699
  • 10
  • 43
  • 56

3 Answers3

2

If the LinkedlIst was constructed in another thread, a read to a volatile is required to produce a read barrier (assume a thread safe means of passing between threads was not used) To be safe, the write need to be performed last to see all writes which occurred be fore.

You need a combination of the first to trigger a volatile read before the list is copied. and second to trigger a volatile write last.

Assuming your QuntityPrice doesn't change this will work.

BTW Using ArrayList is likely to be much faster.

BTW2 even faster would be trying to find a way to avoid creating objects, but there is not enough code to work out how to do that.

Peter Lawrey
  • 525,659
  • 79
  • 751
  • 1,130
  • Are you sure that the semantics of the constructor allow for all the adds to be visible to other threads? You don't happen to know if where this is stated in the JLS? – richs Jun 30 '14 at 18:42
  • @richs AFAIK the volatile write in theory can race with the initial default store. So either a calling thread may see a non-null ProductPrice instance with a null `quantityPriceList` – John Vint Jun 30 '14 at 19:14
  • @richs A volatile write is visible to other threads however the order matters, as does the reads. I have updated my answer. – Peter Lawrey Jun 30 '14 at 19:37
2

Is it correct that the items that I added in my constructor to my volatile linked list below may not be visible to other threads

If the list is non-null, then all the writes to the list will be visible. There is a race condition in which the default write (null) may be visible to a thread seeing a non-null ProductPrice. This isn't true if the field is final.

and in this case could i just make the variable final and get the same effect of having all items visible to other threads.

Yes, this is the best solution.

John Vint
  • 39,695
  • 7
  • 78
  • 108
  • race condition? does that means the state of the linked list is eventually published to all CPU caches, but not guaranteed to be immediate? – richs Jun 30 '14 at 20:00
  • What I mean by that, is if you look at the list before you assign it a `new` variable it will obviously be null. The JMM allows that null value to be visible to other threads even after the construction is finished. – John Vint Jun 30 '14 at 20:33
  • If that list is non-null then all content that was added to the list in the constructor **will** be visible. – John Vint Jun 30 '14 at 20:33
  • Even though the list is volatile. I would think the null value wouldn't be visible to other threads after construction. – richs Jun 30 '14 at 20:37
  • Me neither :) Though it is unlikely programmers cannot rely on the fact. http://cs.oswego.edu/pipermail/concurrency-interest/2013-November/011951.html – John Vint Jul 01 '14 at 12:59
1

If the quantityPriceList variable truly can be final, then the best solution is to declare it as final an populate it through use of the constructor which takes a collection.

class ProductPrice {
  private final LinkedList<QuantityPrice> quantityPriceList;
  public ProductPrice(List<QuantityPrice> quantityPriceListParam) {
      this.quantityPriceList = new LinkedList<QuantityPrice>(quantityPriceListParam);
  }
}

The first example you provided does have the possibility of a concurrency issue, depending on how the availability of your ProductPrice object is published. This is very similar to the existing question.

As @PeterLawrey noted, using an ArrayList is almost certainly a better choice than a LinkedList in this case where you know exactly the number of items to be placed into the list.

Community
  • 1
  • 1
Brett Okken
  • 6,210
  • 1
  • 19
  • 25