On 07/02/2009, Oleg Kalnichevski <[email protected]> wrote: > 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?
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. > Oleg > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
