RE: Thread-safety in Guard

[Jerome Louvel]

Hi Tim,

Thanks again for the feed-back, I've reopened the issue and added a comment
to your blog post.

Best regards,
Jerome  

> -Message d'origine-
> De : [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] De la 
> part de Tim Peierls
> Envoyé : lundi 5 novembre 2007 21:34
> À : discuss@restlet.tigris.org
> Objet : Re: Thread-safety in Guard
> 
> On 11/4/07, Jerome Louvel <[EMAIL PROTECTED]> wrote:
> 
>   I've fixed the remaining Guard.secrets issue following 
> Tim advises:
> - removed the setSecrets() method in trunk
> - eagerly instantiate the secrets map
>   
>   I've also fixed potential issues with RouteList using 
> an underlying CopyOnWriteArrayList instance. 
>   
>   If you see remaining threading issues, please reopen: 
> http://restlet.tigris.org/issues/show_bug.cgi?id=368
>   
> 
> 
> 
> I don't think I have permission to re-open it, but I added 
> the following comment: 
> 
> 
> 
> 
>   The last set of changes is definitely an improvement, 
> but there are still a lot of outstanding concurrency problems. 
>   
>   The secrets field of Guard should be final.
>   
>   In the Restlet class, the context field is not always 
> accessed with appropriate synchronization. 
>   
>   None of the fields of the other classes in org.restlet 
> are accessed with appropriate synchronization.
>   
> 
> 
> 
> I haven't looked at other packages yet.
> 
> I posted a longer discussion of how these problems might be 
> addressed on my blog: 
> 
> http://tembrel.blogspot.com/2007/11/concurrency-issues-in-restlet.html
> 
> --tim
> 
> 

Re: Thread-safety in Guard

[Tim Peierls]

On 11/4/07, Jerome Louvel <[EMAIL PROTECTED]> wrote:
>
> I've fixed the remaining Guard.secrets issue following Tim advises:
>   - removed the setSecrets() method in trunk
>   - eagerly instantiate the secrets map
>
> I've also fixed potential issues with RouteList using an underlying
> CopyOnWriteArrayList instance.
>
> If you see remaining threading issues, please reopen:
> http://restlet.tigris.org/issues/show_bug.cgi?id=368
>


I don't think I have permission to re-open it, but I added the following
comment:


The last set of changes is definitely an improvement, but there are still a
> lot of outstanding concurrency problems.
>
> The secrets field of Guard should be final.
>
> In the Restlet class, the context field is not always accessed with
> appropriate synchronization.
>
> None of the fields of the other classes in org.restlet are accessed with
> appropriate synchronization.
>


I haven't looked at other packages yet.

I posted a longer discussion of how these problems might be addressed on my
blog:

http://tembrel.blogspot.com/2007/11/concurrency-issues-in-restlet.html

--tim

Re: Thread-safety in Guard

[Jerome Louvel]

Hi all,

I've fixed the remaining Guard.secrets issue following Tim advises:
 - removed the setSecrets() method in trunk
 - eagerly instantiate the secrets map

I've also fixed potential issues with RouteList using an underlying 
CopyOnWriteArrayList instance.


If you see remaining threading issues, please reopen:
http://restlet.tigris.org/issues/show_bug.cgi?id=368

Best,
Jerome

Jerome Louvel a écrit :

Hi Tim,

What's the best way for me (or anyone else) to report other 
concurrency and documentation issues that I encounter? In 
this discussion group? This thread? Or somewhere else? 


For issues similar to the one we discussed, the best place is in the issue,
via the comments system. I've just broaden the scope of this issue:

"Fix thread safety issues"
http://restlet.tigris.org/issues/show_bug.cgi?id=368

If you have time to contribute patches, that would be more than welcome. 
 
Here's my understanding of the basic concurrency requirements 
of the Restlet framework: Restlets of all flavors must be 
@ThreadSafe because they will in general be accessed 
concurrently by many threads. Requests and Responses are 
@NotThreadSafe; their handling is confined to a single 
thread. Representations might or might not be thread-safe, 
depending on whether they will be re-used in subsequent 
request handling. Does that sound right? 


It is mostly right. For Representations, I don't think they should be shared
by multiple threads. The instances are generally short-lived, bounded to the
Request/Response life cycle.
 
I posted some basic rules for writing @ThreadSafe classes 
  on my blog. I hope they're helpful.


Cool, I've added the link to the threading documentation RFE:
http://www.outerthought.com/

Best regards,
Jerome  

RE: Thread-safety in Guard

[Jerome Louvel]

Hi Tim,

> What's the best way for me (or anyone else) to report other 
> concurrency and documentation issues that I encounter? In 
> this discussion group? This thread? Or somewhere else? 

For issues similar to the one we discussed, the best place is in the issue,
via the comments system. I've just broaden the scope of this issue:

"Fix thread safety issues"
http://restlet.tigris.org/issues/show_bug.cgi?id=368

If you have time to contribute patches, that would be more than welcome. 
 
> Here's my understanding of the basic concurrency requirements 
> of the Restlet framework: Restlets of all flavors must be 
> @ThreadSafe because they will in general be accessed 
> concurrently by many threads. Requests and Responses are 
> @NotThreadSafe; their handling is confined to a single 
> thread. Representations might or might not be thread-safe, 
> depending on whether they will be re-used in subsequent 
> request handling. Does that sound right? 

It is mostly right. For Representations, I don't think they should be shared
by multiple threads. The instances are generally short-lived, bounded to the
Request/Response life cycle.
 
> I posted some basic rules for writing @ThreadSafe classes 
>  e-classes.html>  on my blog. I hope they're helpful.

Cool, I've added the link to the threading documentation RFE:
http://www.outerthought.com/

Best regards,
Jerome  

Re: Thread-safety in Guard

[Tim Peierls]

On 10/9/07, Jerome Louvel <[EMAIL PROTECTED]> wrote:
>
> Good idea, I've entered a RFE for this documentation task:
> http://restlet.tigris.org/issues/show_bug.cgi?id=369
>

What's the best way for me (or anyone else) to report other concurrency and
documentation issues that I encounter? In this discussion group? This
thread? Or somewhere else?

Here's my understanding of the basic concurrency requirements of the Restlet
framework: Restlets of all flavors must be @ThreadSafe because they will in
general be accessed concurrently by many threads. Requests and Responses are
@NotThreadSafe; their handling is confined to a single thread.
Representations might or might not be thread-safe, depending on whether they
will be re-used in subsequent request handling. Does that sound right?

I posted some basic rules for writing @ThreadSafe
classeson
my blog. I hope they're helpful.

--tim

RE: Thread-safety in Guard

[Jerome Louvel]

Hi Sean,

Good idea, I've entered a RFE for this documentation task:
http://restlet.tigris.org/issues/show_bug.cgi?id=369

Best regards,
Jerome  

> -Message d'origine-
> De : news [mailto:[EMAIL PROTECTED] De la part de Sean Landis
> Envoyé : lundi 8 octobre 2007 20:17
> À : discuss@restlet.tigris.org
> Objet : Re: Thread-safety in Guard
> 
> > Funny you should ask! I can highly recommend Java 
> Concurrency in Practice by
> Brian Goetz, Josh Bloch, Joe Bowbeer, David Holmes, Doug Lea, 
> and me (Tim
> Peierls). My recommendation is probably biased by the fact 
> that I helped write
> it, but we've had a lot of good feedback.
> > --tim
> 
> It is a great book - probably destined to be a classic. 
> Thanks Tim and the 
> rest. 
> 
> The Restlet build uses some good tools to help maintain 
> quality. I'd add 
> another. In appendix A of "Java Concurrency in Practice," 
> Annotations for
> Concurrency is introduced. The primary premise for these - I 
> think - is 
> that JavaDoc doesn't do a good job at expressing the 
> concurrency constraints
> of the code. Not only are these great for documentation 
> purposes, but FindBugs does some checking based on these.
> 
> Sean
> 
> 

Re: Thread-safety in Guard

[Sean Landis]

> Funny you should ask! I can highly recommend Java Concurrency in Practice by
Brian Goetz, Josh Bloch, Joe Bowbeer, David Holmes, Doug Lea, and me (Tim
Peierls). My recommendation is probably biased by the fact that I helped write
it, but we've had a lot of good feedback.
> --tim

It is a great book - probably destined to be a classic. Thanks Tim and the 
rest. 

The Restlet build uses some good tools to help maintain quality. I'd add 
another. In appendix A of "Java Concurrency in Practice," Annotations for
Concurrency is introduced. The primary premise for these - I think - is 
that JavaDoc doesn't do a good job at expressing the concurrency constraints
of the code. Not only are these great for documentation 
purposes, but FindBugs does some checking based on these.

Sean

RE: Thread-safety in Guard

[Jerome Louvel]

Hi Tim,

Great feed-back. I also need a refresher on these issues and just ordered
the book. I've also added it to the recommended books section:
http://www.restlet.org/documentation/books

Also, I've entered a bug report for this thread safety issue:
http://restlet.tigris.org/issues/show_bug.cgi?id=368

Best regards,
Jerome  

> -Message d'origine-
> De : John D. Mitchell [mailto:[EMAIL PROTECTED] 
> Envoyé : dimanche 7 octobre 2007 01:30
> À : discuss@restlet.tigris.org
> Objet : Re: Thread-safety in Guard
> 
> Hey Tim!
> 
> On 10/6/07, Tim Peierls <[EMAIL PROTECTED]> wrote:
> [...]
> > > 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?
> >
> > Funny you should ask! I can highly recommend Java 
> Concurrency in Practice by
> > Brian Goetz, Josh Bloch, Joe Bowbeer, David Holmes, Doug 
> Lea, and me (Tim
> > Peierls). My recommendation is probably biased by the fact 
> that I helped
> > write it, but we've had a lot of good feedback.
> 
> I can also wholeheartedly recommend this book.  I.e., if you do any
> non-trivial Java programming then you should read this book.
> 
> Rock on,
> John

Re: Thread-safety in Guard

[John D. Mitchell]

Hey Tim!

On 10/6/07, Tim Peierls <[EMAIL PROTECTED]> wrote:
[...]
> > 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?
>
> Funny you should ask! I can highly recommend Java Concurrency in Practice by
> Brian Goetz, Josh Bloch, Joe Bowbeer, David Holmes, Doug Lea, and me (Tim
> Peierls). My recommendation is probably biased by the fact that I helped
> write it, but we've had a lot of good feedback.

I can also wholeheartedly recommend this book.  I.e., if you do any
non-trivial Java programming then you should read this book.

Rock on,
John

Re: Thread-safety in Guard

[Tim Peierls]

On 10/6/07, Rob Heittman <[EMAIL PROTECTED]> wrote:
>
> 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?
>

Funny you should ask! I can highly recommend Java Concurrency in
Practiceby Brian Goetz, Josh Bloch, Joe Bowbeer,
David Holmes, Doug Lea, and me (Tim
Peierls). My recommendation is probably biased by the fact that I helped
write it, but we've had a lot of good feedback.

--tim

Re: Thread-safety in Guard

[Rob Heittman]

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: discuss@restlet.tigris.org 
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 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 getSecrets() { 
if ( this.secrets == null) { 
synchronized (this) { 
if (this.secrets == null) { 
this.secrets = new ConcurrentHashMap(); 
} 
} 
} 

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 

Re: Thread-safety in Guard

[Tim Peierls]

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 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 getSecrets() {
> if (this.secrets == null) {
> synchronized (this) {
> if (this.secrets == null) {
> this.secrets = new
> ConcurrentHashMap();
> }
> }
> }
>
> 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

RE: Thread-safety in Guard

[Jerome Louvel]

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 getSecrets() {
if (this.secrets == null) {
synchronized (this) {
if (this.secrets == null) {
this.secrets = new
ConcurrentHashMap();
}
}
}

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
> À : discuss@restlet.tigris.org
> 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 
> 
>  . 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 getSecrets() {
> return secrets;
> }
> 
> private final ConcurrentMap secrets =
> new ConcurrentHashMap();
> }
> 
> 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
> 
>