[concurrency-interest] concurrency newbie question

Ashley Williams ashpublic at mac.com
Mon Jun 22 17:11:39 EDT 2009


Great, somebody has verified the code which is a good start. Thanks.

Ok I've had another look at the book and I think you are right that I
can just use volatiles if I don't need any of the CAS features of  
atomics.
I guess I have it in my mind to use the final modifier at all costs when
declaring fields.

On 22 Jun 2009, at 21:49, Marcelo Fukushima wrote:

> 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