[concurrency-interest] concurrency newbie question

Tim Peierls tim at peierls.net
Tue Jun 23 09:58:03 EDT 2009


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/86b2dae6/attachment-0001.html>


More information about the Concurrency-interest mailing list