[concurrency-interest] concurrency newbie question

Tim Peierls tim at peierls.net
Mon Jun 22 19:36:50 EDT 2009


Maybe we didn't emphasize this enough in JCiP, but it's almost always best
to start with something simple and correct and then move to more complex
designs only if it's clear that you really need to.
Your example looks somewhat artificial, but if I were to take it literally,
I would start with something like this.

public interface StandardSettings {
    boolean isStatusMessage();
    void setStatusMessage(boolean statusMessage);
    ConcurrentMap<String, MachineConfig> getMachines();
    MachineConfig get(String name);
}

@ThreadSafe
public final class StandardSettingsImpl implements StandardSettings {

       @GuardedBy("this") private boolean statusMessage = true;

       private final ConcurrentMap<String, MachineConfig>> machines =
           new ConcurrentHashMap<String, MachineConfig>();


       public synchronized boolean isStatusMessage() {
           return statusMessage;
       }

       public synchronized void setStatusMessage(boolean statusMessage) {
           this.statusMessage = statusMessage;
       }

       public ConcurrentMap<String, MachineConfig> getMachines() {
           return machines;
       }

       public MachineConfig getMachine(String name) {
           return machines.get(name);
       }
}

Prefer the interface type ConcurrentMap (or just Map) in APIs, only using
concrete types like ConcurrentHashMap in initializers.

If you're willing to expose the map from machine name to machine config, you
don't need a setMachines method at all. See chapter 4 of JCiP for variations
on this theme.

Use the JCiP annotations (or similar comments) to document your concurrency
policy.

Don't be shy about using simple synchronization.

--tim



On Mon, Jun 22, 2009 at 4:21 PM, Ashley Williams <ashpublic at mac.com> wrote:

> Hi,
>
> After reading java concurrency in practice, I've now realize how little I
> know after all these years so I'm trying to put that
> right. So as a first step I would like to make a settings class, which is
> just a bunch of getters and setters, a little more
> threadsafe and so I would appreciate any helpful comments.
>
>
> I think I am right in just using a final reference to an atomic
> wrapper for the simple property types, but I'm unsure if I'm approaching
> the map property correctly. In the code below
> I've tried to follow these guidelines for the 'machines' field:
>
> 1. The getter should always return a copy.
>
> 2. The setter should replace the currently held list, rather than trying to
> clear() then putAll() which can cause a race condition
> if another thread gets in between the two operations.
>
> 3. The accessor getMachine(key) method effectively does a double check to
> decide whether it needs to create the entry. The
> first check is against null and the second is an implicit check during the
> call to putIfAbsent().
>
> 4. My first attempt didn't wrap the map with an atomic, instead the map was
> declared directly as a field. I rejected this because
> the set method involved calling clear() then putAll() which can cause a
> race condition
> if another thread gets in between the two operations. Was I right to reject
> this approach?
>
>
> public final class StandardSettings {
>        private final AtomicBoolean statusMessage;
>
>        private final AtomicReference<ConcurrentHashMap<String,
> MachineConfig>> machines;
>
>        public StandardSettings() {
>                this.statusMessage = new AtomicBoolean(true);
>                this.machines = new
> AtomicReference<ConcurrentHashMap<String, MachineConfig>>(
>                                new ConcurrentHashMap<String,
> MachineConfig>());
>        }
>
> // getter and setter for a boolean property
>
>        public Boolean getStatusMessage() {
>                return statusMessage.get();
>        }
>
>        public void setStatusMessage(Boolean statusMessage) {
>                this.statusMessage.set(statusMessage);
>        }
>
> // getter and setter for map property
>
>        public ConcurrentHashMap<String, MachineConfig> getMachines() {
>                return new ConcurrentHashMap<String,
> MachineConfig>(machines.get());
>        }
>
>        public void setMachines(ConcurrentHashMap<String, MachineConfig>
> machines) {
>                this.machines.set(machines);
>        }
>
> // access to the named map entry that creates that entry if it doesn't
> exist
>
>        public MachineConfig getMachine(String name) {
>                MachineConfig config = machines.get().get(name);
>
>                if (config == null) {
>                        MachineConfig newValue = new MachineConfig(name);
>                        // another thread may have just added after our null
> check
>                        MachineConfig oldValue =
> machines.get().putIfAbsent(name, newValue);
>                        config = oldValue != null ? oldValue : newValue;
>
>                }
>
>                return config;
>        }
>
> }
>
> Am I on the right track for making this class thread safe and if not how
> should I be approaching the problem?
>
> Many thanks
> - Ashley Williams
> _______________________________________________
> 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/20090622/3016d0fd/attachment-0001.html>


More information about the Concurrency-interest mailing list