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<http://www.cs.umd.edu/%7Epugh/java/memoryModel/DoubleCheckedLocking.html>,
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
