On Thu, Apr 23, 2009 at 12:44:41PM +0100, sebb wrote: > On 23/04/2009, Oleg Kalnichevski <[email protected]> wrote: > > 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. > > 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: [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] > > > > > > --------------------------------------------------------------------- > 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]
