sebb wrote:
On 03/02/2009, Oleg Kalnichevski <[email protected]> wrote:
On Tue, 2009-02-03 at 14:29 +0000, sebb wrote:
> On 03/02/2009, Oleg Kalnichevski <[email protected]> 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: [email protected]
For additional commands, e-mail: [email protected]
