[concurrency-interest] question/bug in JCiP: Listing 4.12 ?

Tim Peierls tim at peierls.net
Wed Apr 7 14:14:39 EDT 2010


It would be OK to initialize the locations field with a HashMap rather than
a ConcurrentHashMap.

I can't remember why we did it the way we did, but it was probably to avoid
having to explain that HashMap is sufficient but fragile -- you can't
provide methods to add or remove vehicles, for example -- since that's not
the main point of the listing. But now that it has confused at least one
person, it should probably be a footnote.

--tim


On Wed, Apr 7, 2010 at 1:11 PM, Mike <mboard182 at gmail.com> wrote:

> Hi,
>
> In working through Java Concurrency in Practice, I have struggled to
> understand a detail of Listing 4.12.  After researching it, I am wondering
> if I've found a problem.
>
> Specifically, it seams that the 'locations' map should not require
> internal synchronization, since it is accessed only by readers.
>
> This is an 'overkill' bug, not a true flaw, but I feel it is important as I
> am trying to understand exactly what the minimal synchronization approach is
> to provide the class' @ThreadSafe guarantee.
>
> Note: everyone has slightly different terminology, I've attempted to quote
> "" phrases I'm defining per JCiP.  Also, I've included relevant code at the
> end.
>
>  -PublishingVehicleTracker ('PVT' in my text below) is properly constructed
> (no escaped this, etc)
> -after construction its state cannot be modified, so PVT is logically
> immutable.  ("effectively immutable")
> --PVT's state is defined as the keys and *handles* to SafePoints only
> --while the map itself is obviously mutable, it is only published through
> an immutable wrapper
> -access to the state of the SafePoint instances in the Map is handled via
> "delegation"
> -PVT's design (as writen, at least) imposes no invariants or restrictions
> on valid state transitions for SafePoint instances
>
> Therefore, after client code "safely publishes" an instance of PVT, it
> seams to me that access to its underlying Map is threadsafe even if
> 'locations' were just a HashMap.
>
> It seems obvious that since access to 'locations' is read-only, there are
> no race conditions  --  which leaves memory visibility as the issue.   think
> the root of my confusion here is precisely how is visibility guaranteed.  Is
> it from the client of PVT safely publishing the PVT instance?
>
> It occurs to me that I am also relying on the assumption that the
> underlying hashtable is deterministic and 'un'-lazy in its internals -
> technically I guess there could exist an implementation that tunes itself,
> possibly resizing, as it observes usage over time.  Without a syncronized
> map in that case, races would be a possibility.
>
> I tend first assume a problem with my understanding before a bug, so please
> enlighten me here :)  Also, I have a pretty strong C++ background, with some
> understanding of compilers, barriers, etc, if that helps your answers.
>
> Thanks!  -mboard182
>
>
> /////////////begin code
>
> @ThreadSafe
> public class PublishingVehicleTracker {
>
>
> private final Map<String, SafePoint> locations;
>
> private final Map<String, SafePoint> unmodifiableMap;
>
>
> public PublishingVehicleTracker(
>                         Map<String, SafePoint> locations) {
>
>
>     this.locations
>         = new ConcurrentHashMap<String, SafePoint>(locations);
>
>
>     this.unmodifiableMap
>         = Collections.unmodifiableMap(this.locations);
>
> }
>
> public Map<String, SafePoint> getLocations() {
>
>
>     return unmodifiableMap;
> }
>
> public SafePoint getLocation(String id) {
>
>
>     return locations.get(id);
>
> }
>
> public void setLocation(String id, int x, int y) {
>
>
>     if (!locations.containsKey(id))
>
>
>         throw new IllegalArgumentException(
>
>
>             "invalid vehicle name: " + id);
>     locations.get(id).set(x, y);
>
>
>   }
> }
>
> // monitor protected helper-class
>
> @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;
>
> }
>
> }
>
> ///////////end code
>
> _______________________________________________
> Concurrency-interest mailing list
> Concurrency-interest at cs.oswego.edu
> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://cs.oswego.edu/pipermail/concurrency-interest/attachments/20100407/f84510d0/attachment-0001.html>


More information about the Concurrency-interest mailing list