hc-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: svn commit: r767657
Date Thu, 23 Apr 2009 12:10:22 GMT
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.

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

>  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


Mime
View raw message