[concurrency-interest] concurrency newbie question

Ashley Williams ashpublic at mac.com
Tue Jun 23 10:25:45 EDT 2009


Unfortunately that isn't an option for me, firstly because the code  
that loads and saves this class needs
to assume a bean-style interface rather than an intermediate Builder  
class, as it is part of a generic library
that others may use.

Also I've only included a few of the system wide settings there are  
potentially many more and therefore
it would be wasteful that somebody who just wants to change one of the  
dozens of settings
causes a brand new object to be created.

I'm open to the fact that I'm not yet thinking correctly when it comes  
to concurrency ;) but the settings class
I have seems to be fundamentally mutable in nature.

On 23 Jun 2009, at 14:58, Tim Peierls wrote:

> Consider using the builder pattern so that your StandardSettings  
> class can be immutable. See Effective Java, 2nd edition, Item 2.  
> Then you can just provide the settings with a volatile reference to  
> a StandardSettings instance.
>
> --tim
>
> On Tue, Jun 23, 2009 at 9:47 AM, Ashley Williams <ashpublic at mac.com>  
> wrote:
> 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/11ed6af5/attachment-0001.html>


More information about the Concurrency-interest mailing list