On 23/04/2009, Oleg Kalnichevski <[email protected]> wrote: > On Thu, Apr 23, 2009 at 12:28:43AM +0100, sebb wrote: > > On 22/04/2009, [email protected] <[email protected]> wrote: > > > Author: olegk > > > Date: Wed Apr 22 21:03:47 2009 > > > New Revision: 767657 > > > > > > URL: http://svn.apache.org/viewvc?rev=767657&view=rev > > > Log: > > > Made class thread-safe; cleaned up javadoc > > > Modified: > > > > httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/SingleClientConnManager.java > > > > > > > I'm not convinced that the class is thread-safe. > > > > uniquePoolEntry is assigned in the constructor, but is not final, so > > is not necessarily published safely. > > Sebastian > > I always assumed all instance would end up published by a constructor, > otherwise some of them could be left uninitialized (null), but I am not going > to argue about as I simply do not know it for sure.
AIUI the constructor is not special in terms of the memory model, it's just another method. That is why the java.lang.String fields are now final in Java 1.5+ > Also revokeConnection() accesses > > it but is not synchronized. I think this can be fixed by making the > > field final and synching revokeConnection(). > > > > alwaysShutDown is not final, and is not volatile so the constructor > > does not safely publish it. I think it could be made final. > > > > Unless there are objections, I propose to make the above changes. > > No objections here. > > > > > Most of the fields are protected rather than private; this makes it > > possible for sub-classes to bypass thread-safety requirements. It > > seems to me that fields (especially mutable ones) should default to > > the minimum visibility possible, unless it is essential that they > > accessible externally. There has yet to be a GA release, so now is the > > time to lock things down before they "escape". > > > > Making fields private will change the API, so I think that needs a bit > > more discussion. > > Roland left this class open for extension and it is too late to change that. > The question is whether it is worth breaking full compatibility with Android > because of that and getting rapped by Google folks for not sticking to our > part > of the deal? However, has anyone actually used the protected variables? > Oleg > > > > > S/// > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: [email protected] > > For additional commands, e-mail: [email protected] > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
