Very interesting reference about the double checked locking idiom. I was unaware of the problems, and had actually read some of the papers advocating it.
I do a lot of multithreaded Java work in critical environments, and probably need a refresher on these issues. Can you recommend any good books or online resources that may debunk other conventional thread-safety wisdom in light of modern processing architecture and the latest Java memory model revisions? ----- Original Message ----- From: "Tim Peierls" <[EMAIL PROTECTED]> To: [email protected] Sent: Saturday, October 6, 2007 2:53:41 PM (GMT-0500) America/New_York Subject: Re: Thread-safety in Guard On 10/6/07, Jerome Louvel < [EMAIL PROTECTED] > wrote: I have made the following changes to Guard in SVN 1.0 branch: - secrets is now a ConcurrentHashMap instance - getSecrets() lazily instantiates the secrets field in a synchronized block: Unfortunately, unless *all* access to the secrets field is done inside "synchronized (this) {...}" -- including all read access -- this code is still not correct. I notice that you expose a setSecrets method in 1.1 -- since you're exposing the map itself with getSecrets, there is no need for this method; it's not clear what its behavior should be in the presence of concurrent access to the secrets field. I think it would be better to make secrets final and not expose setSecrets -- or if you are already committed to exposing it, implement it as something like public void setSecrets(Map<String, char[]> secrets) { this.secrets.clear(); this.secrets.putAll(secrets); } Interleaved calls to setSecrets might not behave the way you expect -- but you probably shouldn't be making concurrent calls to setSecrets anyway. public Map<String, char[]> getSecrets() { if ( this.secrets == null) { synchronized (this) { if (this.secrets == null) { this.secrets = new ConcurrentHashMap<String, char[]>(); } } } return this.secrets; } This is known as the double-checked locking idiom , and it is broken. Don't use it. In addition, the getSecrets() method in SVN trunk (future 1.1) returns a ConcurrentMap instance instead of just Map. Cool. Also the "scheme" and "realm" properties have setters in 1.1, so no reason to mark them final. But in that case they need to be properly synchronized. In this case, you could make the fields volatile. The other way is to make sure that they are accessed only while holding a synch lock (the same lock each time). Leaving them non-final is not correct. --Tim Peierls
