Hi Tim,
Excellent remarks.
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:
public Map<String, char[]> getSecrets() {
if (this.secrets == null) {
synchronized (this) {
if (this.secrets == null) {
this.secrets = new
ConcurrentHashMap<String, char[]>();
}
}
}
return this.secrets;
}
In addition, the getSecrets() method in SVN trunk (future 1.1) returns a
ConcurrentMap instance instead of just Map. Also the "scheme" and "realm"
properties have setters in 1.1, so no reason to mark them final.
Best regards,
Jerome
> -----Message d'origine-----
> De : [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] De la
> part de Tim Peierls
> Envoyé : jeudi 4 octobre 2007 07:16
> À : [email protected]
> Objet : Thread-safety in Guard
>
> There is a serious thread-safety problem in org.restlet.Guard
> (Restlet 1.0.5). The secrets field is lazily initialized
> without proper synchronization and the TreeMap used to
> initialize it is not thread-safe
> <http://java.sun.com/javase/6/docs/api/java/util/TreeMap.html>
> . The simplest fix would be to use an eagerly-initialized
> ConcurrentHashMap.
>
> In addition, the scheme and realm fields should be marked
> final. They should probably be protected, too, instead of private.
>
> If you want a thread-safe version of Guard without having to
> wait for a fix, you could use something like this:
>
> public class SafeGuard extends Guard {
>
> ...
>
> @Override public ConcurrentMap<String, char[]> getSecrets() {
> return secrets;
> }
>
> private final ConcurrentMap<String, char[]> secrets =
> new ConcurrentHashMap<String, char[]>();
> }
>
> Using a covariant return type of ConcurrentMap allows callers
> to use methods like putIfAbsent, which might be useful in an
> environment where the logins and secrets are very dynamic.
>
> --tim
>
>
