hc-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: Non-threadsafe methods in HC 4
Date Sat, 14 Feb 2009 19:43:24 GMT
On 09/02/2009, sebb <sebbaz@gmail.com> wrote:
> On 07/02/2009, Oleg Kalnichevski <olegk@apache.org> wrote:
>  > sebb wrote:
>  >
>  > > On 03/02/2009, Oleg Kalnichevski <olegk@apache.org> wrote:
>  > >
>  > > > On Tue, 2009-02-03 at 14:29 +0000, sebb wrote:
>  > > >  > On 03/02/2009, Oleg Kalnichevski <olegk@apache.org> wrote:
>  > > >  > > On Mon, 2009-02-02 at 22:44 +0000, sebb wrote:
>  > > >  > >  > Protocol Interceptors are supposed to be thread-safe.
>  > > >  > >  >
>  > > >  > >  > However, BasicHttpProcessor is not thread-safe as changes
to the
>  > Lists
>  > > >  > >  > are not synchronized.
>  > > >  > >  >
>  > > >  > >  > The rest of the Interceptors have no fields so are immutable.
>  > They
>  > > >  > >  > operate on non-thread-safe classes, however I assume
that such
>  > classes
>  > > >  > >  > are confined to a single thread.
>  > > >  > >  >
>  > > >  > >  > ==
>  > > >  > >  >
>  > > >  > >  > SessionRequestImpl is supposed to be thread-safe, however
there
>  > are
>  > > >  > >  > several mutable fields that are not synch or volatile.
>  > > >  > >  >
>  > > >  > >  > ==
>  > > >  > >  >
>  > > >  > >  > Are HttpParams supposed to be shared between threads?
The
>  > Javadoc say
>  > > >  > >  > that they are intended to be immutable once initialised;
however
>  > this
>  > > >  > >  > does not guarantee that the values will be visible to
all
>  > threads.
>  > > >  > >  >
>  > > >  > >  > BasicHttpParams could be made less thread unsafe by
making the
>  > map
>  > > >  > >  > final, and clear()ing rather than null-ing it.
>  > > >  > >  >
>  > > >  > >  > Likewise, if there was a copy constructor, if could
save the map
>  > > >  > >  > variable after creating it.
>  > > >  > >  >
>  > > >  > >
>  > > >  > >
>  > > >  > > (1) Both BasicHttpParams and BasicHttpProtocols can be shared
>  > between
>  > > >  > >  threads but they are expected to be used in the 'write once
/ read
>  > many'
>  > > >  > >  mode. Therefore, access to their internal structures is not
>  > synchronized
>  > > >  > >  for performance reason. These classes are not fully thread-safe
if
>  > > >  > >  modified at runtime and any statement to the contrary is
a mistake
>  > in
>  > > >  > >  documentation.
>  > > >  >
>  > > >  > I think we are talking about two different kinds of thread-safety
>  > here
>  > > >  > - mutual exclusion and data visibility.
>  > > >  >
>  > > >  > I agree that if the classes are used in write once mode, then the
>  > > >  > question of mutual exclusion does not arise.
>  > > >  >
>  > > >  > However, I'm referring to data visibility, which is about ensuring
>  > > >  > that all threads see the same values.
>  > > >  >
>  > > >  > If a thread writes to a field, the updated contents of that field
are
>  > > >  > not necessarily visible to another thread - ever - as the JVM is
free
>  > > >  > to cache values locally.
>  > > >  >
>  > > >
>  > > >
>  > > > Sebastian,
>  > > >
>  > > >  In technical terms variable caching means pretty much one thing only:
>  > > >  the JVM may choose to keep a value of a non-volatile variable in a CPU
>  > > >  register for performance optimization sake. Naturally other threads
>  > > >  would not see that value when the contexts are switched. However, this
>  > > >  can be a problem _only_ if the value stored in a CPU register
>  > _mutates_,
>  > > >  which _should_ never occur as long as BasicHttpParams and
>  > > >  BasicHttpProcessor are used as specified given the fact that register
>  > > >  optimizations are very, very short-lived.
>  > > >
>  > > >
>  > >
>  > > That's probably true for PCs and smaller systems, however larger
>  > > servers often have several caches, some of which may be large and long
>  > > lasting.
>  > >
>  > > For example, I used to work on Alpha systems, and writes to the main
>  > > memory from one CPU could be seen in a different order on another CPU.
>  > > [I understand this was done for performance reasons.]
>  > >
>  > > So a thread on one CPU only sees the correct data if both CPUs issue
>  > > the appropriate memory barrier instructions in the correct order.
>  > >
>  > >
>  > > >  > In order for fields to be shared between threads, a field must
be
>  > > >  > correctly published, e.g. by making it final.
>  > > >  >
>  > > >  > >  I suggest we improve the documentation of BasicHttpParams
and
>  > > >  > >  BasicHttpProtocols with regards to their intended usage,
but will
>  > not
>  > > >  > >  make them fully thread-safe. Making those classes less prone
to
>  > > >  > >  synchronization issues would also be welcome.
>  > > >  > >
>  > > >  > >  Would that be okay for you?
>  > > >  >
>  > > >  > If the classes can be changed to use final for all shared variables,
>  > > >  > then that should be enough to ensure that the data is published
OK.
>  > > >  >
>  > > >
>  > > >
>  > > > Internally BasicHttpParams is backed by a HashMap instance,
>  > > >  BasicHttpProcessor is backed by two ArrayList instance. There is no
>  > > >  problem making references to those instances final. However, this will
>  > > >  not guarantee in any way that the internal variables of those instances
>  > > >  are also final. Basically we gain nothing.
>  > > >
>  > > >
>  > >
>  > > Final fields have additional properties - according to Java
>  > > Concurrency In Practice:
>  > >
>  > > "... any variables that can be reached through a final field of a
>  > > properly constructed object ... such as .. the contents of a HashMap
>  > > referenced by a final field are guaranteed to be visible to other
>  > > threads ... so long as the objects are only reachable through final
>  > > fields of the object under construction."
>  > >
>  > > So I think we can gain something.
>  > >
>  > > I agree it's quite unlikely that the data won't be visible, as other
>  > > activities will probably cause memory flushes, but it seems risky to
>  > > me to depend on that.
>  > >
>  > >
>  > > >  Oleg
>  > > >
>  > > >
>  > >
>  >
>  >  Sebastian,
>  >
>  >  (1) Made sure all instance variables in SessionRequestImplare marked as
>  > volatile.
>  >  (2) Documented BasicHttpParams / BasicHttpProcessor as potentially
>  > thread-unsafe.
>  >  (3) Opened this issue for HttpClient
>  >
>  >  https://issues.apache.org/jira/browse/HTTPCLIENT-824
>  >
>  >  Does this address your concerns? Can we proceed with the release?
>
>
> Sorry for the delay in replying. I'm away from base (trying to avoid
>  getting snowed in) and don't have access to my normal system.
>
>  I cannot check if that solves all the problems, but there's no point
>  holding up the release further. At least users have been warned.
>
>  If there are threading issues they will have to be addressed in a later release.
>
>  So +0 from me.
>

I think it would be worth making the HashMap in BasicHttpParams final,
and using clear() rather than setting the Map to null. This would make
it easier to maintain thread-safety later.

Any objections?

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