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 20:19:23 GMT
On 23/04/2009, Oleg Kalnichevski <olegk@apache.org> wrote:
> 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.

Yes, thanks.

However it's just occurred to me that making alwaysShutDown final
changes the API - does it not?

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

Well, java.lang.String variables don't change after construction, but
they are all final in Java 1.5 (apart from hash, which does not
matter, as it will always have the same value).

See Section 3.2 in:

http://www.cs.umd.edu/~pugh/java/memoryModel/jsr133.pdf

and figures 4 and 5.

> Any other behavior just would not make sense.

It is counter-intuitive, but the compiler is allowed to do all sorts
of tricks to improve performance.

Now there are other ways of ensuring that fields are correctly
published - for example starting a thread. The new thread will see the
same values as the original thread at the point the thread was
started. But if either thread subsequently changes a value, it needs
to ensure that the change is published somehow, e.g. with volatile or
synch. set & get.

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

OK.

Hopefully it may not be necessary to do it for all such variables.

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

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org


Mime
View raw message