[concurrency-interest] concurrency newbie question

Marcelo Fukushima takeshi10 at gmail.com
Mon Jun 22 16:49:34 EDT 2009


I think your code looks correctly, but for readability i would:
-make the machines field volatile instead of using AtomicReference
(for readability)
-unless creating a MachineConfig is very expensive, i'd drop the null
check and always create a new MachineConfig like this:

public MachineConfig getMachine(String name) {
   MachineConfig newValue = new MachineConfig(name);
   MachineConfig oldValue = machines.putIfAbsent(name, newValue);
   return oldValue != null ? oldValue : newValue;
}

but i suppose they are a matter of taste

On Mon, Jun 22, 2009 at 5: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
>



-- 
http://mapsdev.blogspot.com/
Marcelo Takeshi Fukushima



More information about the Concurrency-interest mailing list