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:14:00 GMT
On Thu, Apr 23, 2009 at 01:10:22PM +0100, sebb wrote:
> On 23/04/2009, Oleg Kalnichevski <olegk@apache.org> wrote:
> > 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
> 
> OK, I'd forgotten about that.
> 
> I guess we need to run clirr or similar before the next release in
> case any API changes have crept in, for example some non-private
> fields may have been made final.
> 

I do that as a part of the official release process.


> >  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.
> 
> I suppose we could deprecate any methods or fields if we find that
> they need to be changed for thread-safety or other issues, and fix
> them in a later release.
> 

Yes, we certainly can. If you think the class is broken we should simply copy
it under a different name and deprecate the original one.

Oleg


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