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 18:51:33 GMT
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+
> 

Sebastian,

I made #revokeConnection() synchronized and alwaysShutDown final. I hope 
this addresses your concerns, at least partially.

I could not find a definitive statement to that effect but my 
interpretation of JSR 133 and related documents is such that as long as 
non-final variables do not mutate after initial assignment, there should 
be no threading visibility issue. Any other behavior just would not make 
sense.

If you are not sure, feel free to make all non-final variables volatile.

Cheers

Oleg



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


Mime
View raw message