On Thu, Apr 23, 2009 at 12:14:40PM +0100, sebb wrote: > 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?
We simply can't know that. It is certainly a possibility given HttpClient's user base. Oleg > > > 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] > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
