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 12:00:22 GMT
On Thu, Apr 23, 2009 at 12:44:41PM +0100, sebb wrote:
> On 23/04/2009, Oleg Kalnichevski <olegk@apache.org> wrote:
> > 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.
> 
> But we have only made the promise to Android, surely?
> Or can the API now not be changed at all?
> 

No, it can't, because we, as a project, (I know you personally obstained)
committed ourselves to a formal API freeze: 

http://hc.markmail.org/message/qhb4hsctbwh7uvth?q=API+freeze&page=3

The fatal mistake, which I am fully responsible for, has been to have not
spelled out exactly what was meant by that. Google's expectation is a _full_
_binary_ compatibility between this tag [1] and the final 4.0 release, so that
code compiled against any official post 4.0-beta1 release of HttpClient could
run unchanged on Android 1.0 and visa versa. This was certainly not something I
was prepared to commit myself to, but it is too late now. 

Oleg

[1] http://svn.apache.org/repos/asf/httpcomponents/httpclient/tags/4_0_API_FREEZE/



> >
> >  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
> >
> >
> 
> ---------------------------------------------------------------------
> 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