hc-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Oleg Kalnichevski <ol...@apache.org>
Subject Re: Non-threadsafe methods in HC 4
Date Sat, 07 Feb 2009 14:54:16 GMT
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?

Oleg

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


Mime
View raw message