You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "Sebb (JIRA)" <ji...@apache.org> on 2011/02/01 01:29:29 UTC
[jira] Created: (MATH-505) TestUtils is thread-hostile
TestUtils is thread-hostile
---------------------------
Key: MATH-505
URL: https://issues.apache.org/jira/browse/MATH-505
Project: Commons Math
Issue Type: Bug
Affects Versions: 2.1, 1.2
Reporter: Sebb
TestUtils has several mutable static fields which are not synchronised, or volatile.
If one of the fields is updated by thread A, there is no guarantee that thread B will see the full update - it may see a partially updated object.
Furthermore, at least some of the static fields reference a mutable object, which can be changed whilst another thread is using it.
As far as I can tell, this class must only ever be used by a single thread otherwise the results will be unpredictable.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (MATH-505) TestUtils is thread-hostile
Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-505?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12988978#comment-12988978 ]
Phil Steitz commented on MATH-505:
----------------------------------
What fields, exactly?
> TestUtils is thread-hostile
> ---------------------------
>
> Key: MATH-505
> URL: https://issues.apache.org/jira/browse/MATH-505
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 1.2, 2.1
> Reporter: Sebb
>
> TestUtils has several mutable static fields which are not synchronised, or volatile.
> If one of the fields is updated by thread A, there is no guarantee that thread B will see the full update - it may see a partially updated object.
> Furthermore, at least some of the static fields reference a mutable object, which can be changed whilst another thread is using it.
> As far as I can tell, this class must only ever be used by a single thread otherwise the results will be unpredictable.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Updated: (MATH-505) TestUtils is thread-hostile
Posted by "Sebb (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-505?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Sebb updated MATH-505:
----------------------
Affects Version/s: (was: 1.2)
3.0
> TestUtils is thread-hostile
> ---------------------------
>
> Key: MATH-505
> URL: https://issues.apache.org/jira/browse/MATH-505
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 2.1, 3.0
> Reporter: Sebb
> Assignee: Sebb
>
> TestUtils has several mutable static fields which are not synchronised, or volatile.
> If one of the fields is updated by thread A, there is no guarantee that thread B will see the full update - it may see a partially updated object.
> Furthermore, at least some of the static fields reference a mutable object, which can be changed whilst another thread is using it.
> As far as I can tell, this class must only ever be used by a single thread otherwise the results will be unpredictable.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (MATH-505) TestUtils is thread-hostile
Posted by "Sebb (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-505?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12989013#comment-12989013 ]
Sebb commented on MATH-505:
---------------------------
By thread-hostile, I mean that it is not possible in general for two different threads to use the class safely.
If one thread changes any of the static fields, there is no way of knowing how the methods called by the other thread will behave. This is partly because the values are not safely published currently, but even if they were, the threads don't know what settings will be used as they can be changed at any time by another thread.
In general, any class which relies on mutable static state for its behaviour is thread-hostile.
The shared state cannot simultaneously satisfy two threads needing different behaviour.
I think the only safe way for two threads to use the class as it stands is if they both synchronize on the class.
This will ensure safe publication of any field changes, and enforce serial usage which can guarantee the setting that will be used (but the lock will have to be held for the set call as well).
ChiSquareTestImpl has a non-final instance field which means its value won't necessarily be safely published.
The field also has a setter which could be invoked by one thread while another was using it.
TTestImpl is immutable (has no fields), and OneWayAnovaImpl can be made immutable, but other implementations of the interfaces might exist which are not immutable.
The simplest way to make the class thread-safe would be to convert all the methods and fields from static to instance, but I don't know if that is acceptable.
> TestUtils is thread-hostile
> ---------------------------
>
> Key: MATH-505
> URL: https://issues.apache.org/jira/browse/MATH-505
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 1.2, 2.1
> Reporter: Sebb
>
> TestUtils has several mutable static fields which are not synchronised, or volatile.
> If one of the fields is updated by thread A, there is no guarantee that thread B will see the full update - it may see a partially updated object.
> Furthermore, at least some of the static fields reference a mutable object, which can be changed whilst another thread is using it.
> As far as I can tell, this class must only ever be used by a single thread otherwise the results will be unpredictable.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Updated: (MATH-505) TestUtils is thread-hostile
Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-505?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Phil Steitz updated MATH-505:
-----------------------------
Affects Version/s: (was: 3.0)
> TestUtils is thread-hostile
> ---------------------------
>
> Key: MATH-505
> URL: https://issues.apache.org/jira/browse/MATH-505
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 1.2, 2.0, 2.1
> Reporter: Sebb
> Assignee: Sebb
> Fix For: 3.0
>
>
> TestUtils has several mutable static fields which are not synchronised, or volatile.
> If one of the fields is updated by thread A, there is no guarantee that thread B will see the full update - it may see a partially updated object.
> Furthermore, at least some of the static fields reference a mutable object, which can be changed whilst another thread is using it.
> As far as I can tell, this class must only ever be used by a single thread otherwise the results will be unpredictable.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (MATH-505) TestUtils is thread-hostile
Posted by "Sebb (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-505?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12988980#comment-12988980 ]
Sebb commented on MATH-505:
---------------------------
{code}
/** Singleton TTest instance using default implementation. */
private static TTest tTest = new TTestImpl();
/** Singleton ChiSquareTest instance using default implementation. */
private static ChiSquareTest chiSquareTest =
new ChiSquareTestImpl();
/** Singleton ChiSquareTest instance using default implementation. */
private static UnknownDistributionChiSquareTest unknownDistributionChiSquareTest =
new ChiSquareTestImpl();
/** Singleton OneWayAnova instance using default implementation. */
private static OneWayAnova oneWayAnova =
new OneWayAnovaImpl();
{code}
All of the above may be changed by set methods. There is no synch.
> TestUtils is thread-hostile
> ---------------------------
>
> Key: MATH-505
> URL: https://issues.apache.org/jira/browse/MATH-505
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 1.2, 2.1
> Reporter: Sebb
>
> TestUtils has several mutable static fields which are not synchronised, or volatile.
> If one of the fields is updated by thread A, there is no guarantee that thread B will see the full update - it may see a partially updated object.
> Furthermore, at least some of the static fields reference a mutable object, which can be changed whilst another thread is using it.
> As far as I can tell, this class must only ever be used by a single thread otherwise the results will be unpredictable.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Updated: (MATH-505) TestUtils is thread-hostile
Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-505?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Phil Steitz updated MATH-505:
-----------------------------
Affects Version/s: 1.2
2.0
Fix Version/s: 3.0
> TestUtils is thread-hostile
> ---------------------------
>
> Key: MATH-505
> URL: https://issues.apache.org/jira/browse/MATH-505
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 1.2, 2.0, 2.1
> Reporter: Sebb
> Assignee: Sebb
> Fix For: 3.0
>
>
> TestUtils has several mutable static fields which are not synchronised, or volatile.
> If one of the fields is updated by thread A, there is no guarantee that thread B will see the full update - it may see a partially updated object.
> Furthermore, at least some of the static fields reference a mutable object, which can be changed whilst another thread is using it.
> As far as I can tell, this class must only ever be used by a single thread otherwise the results will be unpredictable.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (MATH-505) TestUtils is thread-hostile
Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-505?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12989039#comment-12989039 ]
Phil Steitz commented on MATH-505:
----------------------------------
Making the methods instance sort of defeats the purpose of the class. None of the instance data in any of the static singletons is actually used or depended on by the methods of this class. You are correct though that if one thread changes the impl for one of the singletons while another is using the class, the other could see a different than expected impl. I think the practical likelihood of this is pretty much nil, as it is hard to imagine an application supplying two different implementations for the tests and wanting different threads to use different impls. Personally, I would be happy just documenting the fact that the class is not threadsafe and if concurrent threads want to plug in different implementations, they need to synchronize on the class. If this is not acceptable, my next preference would be to remove the pluggability - i.e., make the singletons final or get rid of them altogether, creating instances as needed for static method calls. There is no initialization overhead creating the test classes.
> TestUtils is thread-hostile
> ---------------------------
>
> Key: MATH-505
> URL: https://issues.apache.org/jira/browse/MATH-505
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 1.2, 2.1
> Reporter: Sebb
>
> TestUtils has several mutable static fields which are not synchronised, or volatile.
> If one of the fields is updated by thread A, there is no guarantee that thread B will see the full update - it may see a partially updated object.
> Furthermore, at least some of the static fields reference a mutable object, which can be changed whilst another thread is using it.
> As far as I can tell, this class must only ever be used by a single thread otherwise the results will be unpredictable.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Resolved: (MATH-505) TestUtils is thread-hostile
Posted by "Sebb (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-505?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Sebb resolved MATH-505.
-----------------------
Resolution: Fixed
> TestUtils is thread-hostile
> ---------------------------
>
> Key: MATH-505
> URL: https://issues.apache.org/jira/browse/MATH-505
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 2.1, 3.0
> Reporter: Sebb
> Assignee: Sebb
>
> TestUtils has several mutable static fields which are not synchronised, or volatile.
> If one of the fields is updated by thread A, there is no guarantee that thread B will see the full update - it may see a partially updated object.
> Furthermore, at least some of the static fields reference a mutable object, which can be changed whilst another thread is using it.
> As far as I can tell, this class must only ever be used by a single thread otherwise the results will be unpredictable.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (MATH-505) TestUtils is thread-hostile
Posted by "Joerg Schaible (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-505?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12989071#comment-12989071 ]
Joerg Schaible commented on MATH-505:
-------------------------------------
@Phil: Please also keep in mind that M3 supports now (currently optional) parallel execution and it might be no longer a proper assumption that all tests are executed serially.
> TestUtils is thread-hostile
> ---------------------------
>
> Key: MATH-505
> URL: https://issues.apache.org/jira/browse/MATH-505
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 1.2, 2.1
> Reporter: Sebb
>
> TestUtils has several mutable static fields which are not synchronised, or volatile.
> If one of the fields is updated by thread A, there is no guarantee that thread B will see the full update - it may see a partially updated object.
> Furthermore, at least some of the static fields reference a mutable object, which can be changed whilst another thread is using it.
> As far as I can tell, this class must only ever be used by a single thread otherwise the results will be unpredictable.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (MATH-505) TestUtils is thread-hostile
Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-505?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12988986#comment-12988986 ]
Phil Steitz commented on MATH-505:
----------------------------------
OK, I was looking at the wrong TestUtils :)
The reason for this strange-looking setup is to allow the implementations to be pluggable at runtime. "Hostile" is a harsh word, but this class is certainly *not* threadsafe. Ideas / patches to achieve the design goal with less "hostility" would be appreciated.
I would have to double-check, but I don't think that there is any test instance state used by the methods in this class.
> TestUtils is thread-hostile
> ---------------------------
>
> Key: MATH-505
> URL: https://issues.apache.org/jira/browse/MATH-505
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 1.2, 2.1
> Reporter: Sebb
>
> TestUtils has several mutable static fields which are not synchronised, or volatile.
> If one of the fields is updated by thread A, there is no guarantee that thread B will see the full update - it may see a partially updated object.
> Furthermore, at least some of the static fields reference a mutable object, which can be changed whilst another thread is using it.
> As far as I can tell, this class must only ever be used by a single thread otherwise the results will be unpredictable.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (MATH-505) TestUtils is thread-hostile
Posted by "Sebb (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-505?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12989188#comment-12989188 ]
Sebb commented on MATH-505:
---------------------------
There is another possible option, which would be to fix the default implementations, and create new static methods that took an extra parameter for the implementation to be used.
At present, changes to the static fields are not guaranteed to be published correctly. Making them volatile would fix this, but would not help with concurrent access.
> TestUtils is thread-hostile
> ---------------------------
>
> Key: MATH-505
> URL: https://issues.apache.org/jira/browse/MATH-505
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 1.2, 2.1
> Reporter: Sebb
>
> TestUtils has several mutable static fields which are not synchronised, or volatile.
> If one of the fields is updated by thread A, there is no guarantee that thread B will see the full update - it may see a partially updated object.
> Furthermore, at least some of the static fields reference a mutable object, which can be changed whilst another thread is using it.
> As far as I can tell, this class must only ever be used by a single thread otherwise the results will be unpredictable.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (MATH-505) TestUtils is thread-hostile
Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-505?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12989232#comment-12989232 ]
Phil Steitz commented on MATH-505:
----------------------------------
Thanks, Joerg. There should be no problems with the unit tests unless and until we introduce different tests that actually test the pluggability.
I thought about the additional parameter option, Sebb; but that again defeats the purpose of this "convenience class" - you might as well just instantiate the implementation and use it.
I think the best solution is to just make the fields final and drop the getters and setters. This is consistent with StatUtils. So we should document the "hostility" issues in 2.2 and deprecate there and drop in 3.0.
> TestUtils is thread-hostile
> ---------------------------
>
> Key: MATH-505
> URL: https://issues.apache.org/jira/browse/MATH-505
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 1.2, 2.1
> Reporter: Sebb
>
> TestUtils has several mutable static fields which are not synchronised, or volatile.
> If one of the fields is updated by thread A, there is no guarantee that thread B will see the full update - it may see a partially updated object.
> Furthermore, at least some of the static fields reference a mutable object, which can be changed whilst another thread is using it.
> As far as I can tell, this class must only ever be used by a single thread otherwise the results will be unpredictable.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira