hc-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Oleg Kalnichevski <ol...@apache.org>
Subject Re: svn commit: r767657
Date Thu, 23 Apr 2009 11:37:04 GMT
On Thu, Apr 23, 2009 at 12:14:40PM +0100, sebb wrote:
> On 23/04/2009, Oleg Kalnichevski <olegk@apache.org> wrote:
> > On Thu, Apr 23, 2009 at 12:28:43AM +0100, sebb wrote:
> >  > On 22/04/2009, olegk@apache.org <olegk@apache.org> 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: dev-unsubscribe@hc.apache.org
> >  > For additional commands, e-mail: dev-help@hc.apache.org
> >  >
> >
> >  ---------------------------------------------------------------------
> >  To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
> >  For additional commands, e-mail: dev-help@hc.apache.org
> >
> >
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
> For additional commands, e-mail: dev-help@hc.apache.org
> 

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org


Mime
View raw message