[concurrency-interest] concurrency newbie question

Ashley Williams ashpublic at mac.com
Mon Jun 22 16:21:23 EDT 2009


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


More information about the Concurrency-interest mailing list