[concurrency-interest] concurrency newbie question

Ashley Williams ashpublic at mac.com
Tue Jun 23 09:47:07 EDT 2009


Hi Tim,

Just to give you a little more background, the settings class supports  
configuration and is designed to be exported and imported
to a properties file. To do this I have a piece of code that is able  
to parse a java bean such as this one and uses introspection to
call its getters and setters at the right time. This happens on a  
timer. This also explains why I need a setter for the map property,
since this is used to assign a newly created map based on the  
properties file contents.

Meanwhile the rest of the code is able to consult the settings at any  
time and therefore I need to make it threadsafe. I've pasted
a fuller version of the class below. I don't need an interface in this  
situation since there is no reason to create any alternative
implementations.

According to the feedback I got from Marcelo, I've changed from  
atomics to volatiles which instantly looks more readable -
but was this a wise decision, since I no longer have any final fields  
and am therefore at risk of having a partially
constructed object? The implications of this is that the defaults  
specified in the constructor won't be assigned in time and
would be a problem were I to make my defaults more meaningful.

So: "never want partially constructed objects -> always use final  
field modifier -> have to use atomics"???

- Ashley


@StandardProjectComponent
@ThreadSafe
public final class StandardSettings {
	/**
	 * See {@link #getStatusWindow()}.
	 */
	private volatile Boolean statusWindow;

	/**
	 * See {@link #getQuiet()}.
	 */
	private volatile Boolean quiet;

	/**
	 * See {@link #getLevel()}.
	 */
	private volatile Level level;

	/**
	 * See {@link #getStatusMessage()}.
	 */
	private volatile Boolean statusMessage;

	/**
	 * See {@link #getCompileSource()}.
	 */
	private volatile String compileSource;

	/**
	 * See {@link #getCompileTarget()}.
	 */
	private volatile String compileTarget;

	/**
	 * See {@link #getCompileBootclasspath()}.
	 */
	private volatile String compileBootclasspath;

	/**
	 * See {@link #getCompileExtdirs()}.
	 */
	private volatile String compileExtdirs;

	/**
	 * See {@link #getCompileMemory()}.
	 */
	private volatile String compileMemory;

	/**
	 * See {@link #getProfileHome()}.
	 */
	private volatile File profileHome;

	/**
	 * See {@link #getProfileUrl()}.
	 */
	private volatile String profileUrl;

	/**
	 * See {@link #getMachine(String)}.
	 */
	private volatile ConcurrentHashMap<String, MachineConfig> machines;

	/**
	 * Sets up default values.
	 */
	public MySettings() {
		this.statusWindow = false;
		this.quiet = true;
		this.statusMessage = true;
		this.level = Level.INFO;
		this.compileMemory = null;
		this.compileSource = null;
		this.compileTarget = null;
		this.compileBootclasspath = null;
		this.compileExtdirs = null;
		this.profileHome = null;
		this.profileUrl = null;
		this.machines = new ConcurrentHashMap<String, MachineConfig>();
	}

	/**
	 * Returns the machine configuration with the given name, creating it  
if it
	 * doesn't exist.
	 *
	 * @param name
	 * @return
	 */
	public MachineConfig getMachine(String name) {
		MachineConfig config = machines.get(name);

		if (config == null) {
			MachineConfig newValue = new MachineConfig(name);
			// another thread may have just added after our null check
			MachineConfig oldValue = machines.putIfAbsent(name, newValue);
			config = oldValue != null ? oldValue : newValue;

		}

		return config;
	}

	/**
	 * Returns a hash map that is a copy of the internal map.
	 *
	 * @return
	 */
	public ConcurrentHashMap<String, MachineConfig> getMachines() {
		return new ConcurrentHashMap<String, MachineConfig>(machines);
	}

	public void setMachines(ConcurrentHashMap<String, MachineConfig>  
machines) {
		this.machines = machines;
	}

	public MachineConfig getJunitMachine() {
		return getMachine("junit");
	}

	public MachineConfig getMainMachine() {
		return getMachine("main");
	}

	public Boolean getStatusWindow() {
		return statusWindow;
	}

	public void setStatusWindow(Boolean statusWindow) {
		this.statusWindow = statusWindow;
	}

	public Boolean getQuiet() {
		return quiet;
	}

	public void setQuiet(Boolean quiet) {
		this.quiet = quiet;
	}

	public Level getLevel() {
		return level;
	}

	public void setLevel(Level level) {
		this.level = level;
	}

	public Boolean getStatusMessage() {
		return statusMessage;
	}

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

	public String getCompileSource() {
		return compileSource;
	}

	public void setCompileSource(String compileSource) {
		this.compileSource = compileSource;
	}

	public String getCompileTarget() {
		return compileTarget;
	}

	public void setCompileTarget(String compileTarget) {
		this.compileTarget = compileTarget;
	}

	public String getCompileBootclasspath() {
		return compileBootclasspath;
	}

	public void setCompileBootclasspath(String compileBootclasspath) {
		this.compileBootclasspath = compileBootclasspath;
	}

	public String getCompileExtdirs() {
		return compileExtdirs;
	}

	public void setCompileExtdirs(String compileExtdirs) {
		this.compileExtdirs = compileExtdirs;
	}

	public String getCompileMemory() {
		return compileMemory;
	}

	public void setCompileMemory(String compileMemory) {
		this.compileMemory = compileMemory;
	}

	public File getProfileHome() {
		return profileHome;
	}

	public void setProfileHome(File profileHome) {
		this.profileHome = profileHome;
	}

	public String getProfileUrl() {
		return profileUrl;
	}

	public void setProfileUrl(String profileUrl) {
		this.profileUrl = profileUrl;
	}

}

On 23 Jun 2009, at 00:36, Tim Peierls wrote:

> 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/20090623/0bf31d2b/attachment-0001.html>


More information about the Concurrency-interest mailing list