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