Re: Non-threadsafe methods in HC 4
sebb wrote: On 09/02/2009, sebb wrote: On 07/02/2009, Oleg Kalnichevski wrote: ... > 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? No objections from me. Please go ahead with the change. Oleg > 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] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: Non-threadsafe methods in HC 4
On 09/02/2009, sebb wrote: > On 07/02/2009, Oleg Kalnichevski wrote: > > sebb wrote: > > > > > On 03/02/2009, Oleg Kalnichevski wrote: > > > > > > > On Tue, 2009-02-03 at 14:29 +, sebb wrote: > > > > > On 03/02/2009, Oleg Kalnichevski wrote: > > > > > > On Mon, 2009-02-02 at 22:44 +, 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
Re: Non-threadsafe methods in HC 4
On 07/02/2009, Oleg Kalnichevski wrote: > sebb wrote: > > > On 03/02/2009, Oleg Kalnichevski wrote: > > > > > On Tue, 2009-02-03 at 14:29 +, sebb wrote: > > > > On 03/02/2009, Oleg Kalnichevski wrote: > > > > > On Mon, 2009-02-02 at 22:44 +, 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
Re: Non-threadsafe methods in HC 4
sebb wrote: On 03/02/2009, Oleg Kalnichevski wrote: On Tue, 2009-02-03 at 14:29 +, sebb wrote: > On 03/02/2009, Oleg Kalnichevski wrote: > > On Mon, 2009-02-02 at 22:44 +, 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/H
Re: Non-threadsafe methods in HC 4
Mike Clark wrote: Oleg Kalnichevski wrote: sebb wrote: 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. I only have experience programming in x86 assembly, so I am not in a position to judge. However, this is indeed the case, we should drop any pretense those classes could be used by multiple threads and simply declare them thread-unsafe. Are instances of these classes limited to single threads in the current codebase? Yes, they are. AbstractHttpMessage instantiates BasicHttpParams, which is not a problem, because the class is not thread-safe anyways. All other classes use interfaces, not concrete implementations. Does that need to be reviewed to be confirmed? Extra code review never hurts. Is there any foreseeable need to have instances of these classes shared between threads? In HttpClient, yes. Both parameters and protocol interceptors can be set at the client level and then used by multiple request execution threads. Oleg Mike - 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]
Re: Non-threadsafe methods in HC 4
Oleg Kalnichevski wrote: sebb wrote: 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. I only have experience programming in x86 assembly, so I am not in a position to judge. However, this is indeed the case, we should drop any pretense those classes could be used by multiple threads and simply declare them thread-unsafe. Are instances of these classes limited to single threads in the current codebase? Does that need to be reviewed to be confirmed? Is there any foreseeable need to have instances of these classes shared between threads? Mike - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: Non-threadsafe methods in HC 4
sebb wrote: On 03/02/2009, Oleg Kalnichevski wrote: On Tue, 2009-02-03 at 14:29 +, sebb wrote: > On 03/02/2009, Oleg Kalnichevski wrote: > > On Mon, 2009-02-02 at 22:44 +, 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. I only have experience programming in x86 assembly, so I am not in a position to judge. However, this is indeed the case, we should drop any pretense those classes could be used by multiple threads and simply declare them thread-unsafe. This, however, only affects HttpClient 4.0, but should not delay the release of HttpCore, once the javadocs and the tutorial are updated accordingly. Oleg > 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 unli
Re: Non-threadsafe methods in HC 4
On 03/02/2009, Oleg Kalnichevski wrote: > On Tue, 2009-02-03 at 14:29 +, sebb wrote: > > On 03/02/2009, Oleg Kalnichevski wrote: > > > On Mon, 2009-02-02 at 22:44 +, 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 > > > > > (2) I'll review SessionRequestImpl > > > > > > Oleg > > > > > > > S/// > > > > > > > >
Re: Non-threadsafe methods in HC 4
Oleg Kalnichevski wrote: given the fact that register optimizations are very, very short-lived. Isn't it best not to rely on the duration of register caching, unless it truly isn't important for threads to see up-to-date field values? We see short duration optimizations today... but a year from now? Two years from now? One of the servers I work with has multiple multi-core CPUs and it's a little scary sometimes! 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. This is a good observation. You would need to introduce a JMM memory flush point (e.g. synchronized block) to push out the values of any non-final/non-volatile fields-of-fields-(etc). However, even in the face of this issue, if a field can reasonably be made final/volatile, and that field is shared across threads, even if it does not eliminate all potential caching issues, I think it should still be done, just for good measure, and the potential remaining issues commented in the code. Also, if there are truly known (even if unobserved) issues with fields not being cache-safe, and if those issues would result in incorrect behavior of the code (not just non-performant behavior) it may be most ideal to introduce a JMM memory flush point such as a synchronized collection or manually introducing a synchronized block. Mike - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: Non-threadsafe methods in HC 4
On Tue, 2009-02-03 at 14:29 +, sebb wrote: > On 03/02/2009, Oleg Kalnichevski wrote: > > On Mon, 2009-02-02 at 22:44 +, 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. > 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. Oleg > > (2) I'll review SessionRequestImpl > > > > Oleg > > > > > S/// > > > > > > - > > > 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] > > > > > > - > 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]
Re: Non-threadsafe methods in HC 4
sebb wrote: 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. Or 'volatile'. Although 'final' should be preferred where possible. Mike - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: Non-threadsafe methods in HC 4
On 03/02/2009, Oleg Kalnichevski wrote: > On Mon, 2009-02-02 at 22:44 +, 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. 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. > (2) I'll review SessionRequestImpl > > Oleg > > > S/// > > > > - > > 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] > > - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: Non-threadsafe methods in HC 4
On Mon, 2009-02-02 at 22:44 +, 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 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? (2) I'll review SessionRequestImpl Oleg > S/// > > - > 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]
