DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962 Alex Giotis alex.gio...@gmail.com changed: What|Removed |Added Status|RESOLVED|CLOSED -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962 Vincent Hennebert vhenneb...@gmail.com changed: What|Removed |Added Status|NEW |RESOLVED Resolution||FIXED --- Comment #27 from Vincent Hennebert vhenneb...@gmail.com 2012-03-23 11:30:16 UTC --- I uploaded my changes to the test cases in a new Bugzilla entry, see bug #52977. I'm closing this one as the original problem has been fixed. -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962 --- Comment #23 from Vincent Hennebert vhenneb...@gmail.com 2012-03-22 16:54:42 UTC --- Hi Alexios, (In reply to comment #21) Vincent, thanks for looking at it. * Why use Double.doubleToLongBits in equals methods to compare doubles (NumericProperty, PercentLength)? Why not just thisDouble == otherDouble? (One could also argue that some epsilon might be necessary, but this is another topic.) Well, if we forget about the epsilon, the doubleToLongBits is the correct way to check for equality two doubles. This is well explained in Effective Java by Joshua Bloch, used in Double#compare, Double#equals and by all IDEs that generate them and libraries like Apache commons-lang EqualsBuilder. Java has double values for both 0.0 and -0.0, as well as the never equal not a number (NaN). Those can't be checked with ==. Have a look into the doubleToLongBits source and at the javadoc for Double.equals(). Learning new things every day :-) Thanks for the pointers. * shouldn't the specVal field from Property also be tested for equality? It seems that currently this is not needed. The specVal is set by 3 properties, the FontShorthandProperty, the LineHeightProperty and the URIProperty. None of them uses the cache, so currently it is not needed in the equality tests. Ok, make sense. Still I'll modify the implementations of equals and hashCode in the URIProperty class as it makes explicit use of specVal. Please tell me if you expect an updated patch. It's not necessary. Thanks, Vincent -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962 --- Comment #24 from Vincent Hennebert vhenneb...@gmail.com 2012-03-22 18:00:46 UTC --- Patch applied in rev. 1303891: http://svn.apache.org/viewvc?rev=1303891view=rev Sorry for the delay about this, and thanks for your patience. I didn't include the test cases. They still require quite some work, which is more than I can allocate on this. But most of all, I'm not quite sure that they represent the right approach to the problem. They use a hell lot of mocking which makes them hard to understand, let alone maintain. And despite that, they probably don't even bring adequate coverage. Many fields are set to a mock of some property (see e.g. EnumLengthTestCase). So the equals method will compare two physically identical instances, which is definitely a narrow use case. We should also test the cases of properties made of physically different but logically identical fields. Likewise, the tests for not equals should be broader and test different combinations of field values. Implementing proper coverage would require even more mocking, and things will start to be really unwieldy. I'm not sure what's the right approach to this. Maybe some helper library like EqualsVerifier? http://code.google.com/p/equalsverifier/ Vincent -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962 --- Comment #25 from Alex Giotis alex.gio...@gmail.com 2012-03-22 19:20:08 UTC --- Thanks for reviewing and applying this patch ! I am happy that the deadlock is gone. I had a 2nd look on the final changes and for me this issue is now resolved. I am leaving this bug open to fix the final declaration of CommonBorderPaddingBackground (mockito can't mock it) and in case somebody wants to add explicit tests for the hashCode/equals methods. -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962 --- Comment #26 from Mehdi Houshmand med1...@gmail.com 2012-03-22 21:55:19 UTC --- snip/ And despite that, they probably don't even bring adequate coverage. Many fields are set to a mock of some property (see e.g. EnumLengthTestCase). So the equals method will compare two physically identical instances, which is definitely a narrow use case. We should also test the cases of properties made of physically different but logically identical fields. Likewise, the tests for not equals should be broader and test different combinations of field values. Implementing proper coverage would require even more mocking, and things will start to be really unwieldy. Yup, I came to the same conclusion and did what I could in the limited time I had... Not ideal, but better than nothing right? I'm not sure what's the right approach to this. Maybe some helper library like EqualsVerifier? http://code.google.com/p/equalsverifier/ I did see this and it'd be great if we used this, and it'd be awesome if we used used a repository management system. But a) it introduces a new version of objenesis (which may or may not cause compatibility issues) b) 2 new compile time dependencies c) who's going to create the documentation. Using these new libraries for testing is great, if people know how to use them (and that's a big caveat). Take mocking for example, it's used widely enough in the industry to assume devs either know or can find out how to use the mocking frameworks. I'm not convinced this particular library is widely used enough and if something broke we have to be confident ANYONE can fix the issue. I should say, I'm not against adding these libraries, I think it's important to take testing seriously, but we have to be aware of the implications of adding dependencies and not just in terms of the classpath. -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962 --- Comment #20 from Vincent Hennebert vhenneb...@gmail.com 2012-03-21 20:41:37 UTC --- Thanks for your patch! I have a few questions following a quick review: * Why declare the equals and hashCode methods on interfaces (Numeric, PercentBase)? They are defined on Object anyway, and AFAICT declaring them on interfaces wouldn't change anything (that is, force the developer to implement them on sub-classes?). * Why define those methods as abstract on Property? What if a sub-class is perfectly happy with the default implementations from Object? Also, it doesn't allow to call super.hashCode or super.equals any more. I'm not sure at all if this is desirable. * Why use Double.doubleToLongBits in equals methods to compare doubles (NumericProperty, PercentLength)? Why not just thisDouble == otherDouble? (One could also argue that some epsilon might be necessary, but this is another topic.) * shouldn't the specVal field from Property also be tested for equality? Thanks, Vincent -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962 --- Comment #21 from Alex Giotis alex.gio...@gmail.com 2012-03-22 02:02:02 UTC --- Vincent, thanks for looking at it. * Why declare the equals and hashCode methods on interfaces (Numeric, PercentBase)? They are defined on Object anyway, and AFAICT declaring them on interfaces wouldn't change anything (that is, force the developer to implement them on sub-classes?). The definitions of hashcode/equals on the interfaces are not needed. It was helping me easily locate which classes implement them. Can be removed. * Why define those methods as abstract on Property? What if a sub-class is perfectly happy with the default implementations from Object? Also, it doesn't allow to call super.hashCode or super.equals any more. I'm not sure at all if this is desirable. This was for forcing future sub-classes to implement them as it is not obvious when an implementation is needed. There are properties like ListProperty (a list of Property instances) and classes like the CompoundPropertyMaker which use it. For example, the FontFamilyProperty extends ListProperty and uses the cache. If we rely on the implementations from Object, then the caching is simply an overhead as the FontFamilyProperty#make method will never create two properties with the same identity (Object#equals uses the == operator). Having said that, if this is not desirable, the abstract methods on Property can be removed. * Why use Double.doubleToLongBits in equals methods to compare doubles (NumericProperty, PercentLength)? Why not just thisDouble == otherDouble? (One could also argue that some epsilon might be necessary, but this is another topic.) Well, if we forget about the epsilon, the doubleToLongBits is the correct way to check for equality two doubles. This is well explained in Effective Java by Joshua Bloch, used in Double#compare, Double#equals and by all IDEs that generate them and libraries like Apache commons-lang EqualsBuilder. Java has double values for both 0.0 and -0.0, as well as the never equal not a number (NaN). Those can't be checked with ==. Have a look into the doubleToLongBits source and at the javadoc for Double.equals(). * shouldn't the specVal field from Property also be tested for equality? It seems that currently this is not needed. The specVal is set by 3 properties, the FontShorthandProperty, the LineHeightProperty and the URIProperty. None of them uses the cache, so currently it is not needed in the equality tests. Generally, it should be safe to demand from every class using the cache to correctly implement the hashCode/equal methods. Unfortunately, we can not in general enforce it. With that view, it could be better to remove the abstract declarations on Property. Please tell me if you expect an updated patch. -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962 --- Comment #22 from Alex Giotis alex.gio...@gmail.com 2012-03-22 02:08:31 UTC --- This patch should also resolve Bug 50703. -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962
--- Comment #17 from Mehdi Houshmand med1...@gmail.com 2012-03-09 09:12:12
UTC ---
snip/
- I don't think we get added value by testing the hashCode() equals()
implementations. The tests don't protect from future changes in the tested
classes (e.g. adding a new field) and don't protect if in the future another
class is added in the cache.
While I agree that these tests don't protect from subsequent adding of class
members, I couldn't disagree more that they shouldn't be tested! The equals()
and hashCode() methods have a contract to Object that they should behave in a
certain fashion. If that behaviour is modified, bugs can be hard to track down
and difficult to diagnose because they're so widely used in Java collections
library.
- To find broken hashCode() implementation of cached instances, the
PropertyCache counts the number of collisions and reports it in the log if it
exceeds a number.
Of course anyone is welcomed to add tests, if he wishes so. For completeness,
below is Vincent's test that reveals the JDK5 bug:
private final PropertyCacheInteger cache = new PropertyCacheInteger();
private class CacheFiller implements Runnable {
private final int start;
CacheFiller(int start) {
this.start = start;
}
public void run() {
for (int i = 0; i 100; i++) {
cache.fetch(new Integer(start + i));
}
}
}
public void testCleanUp() throws InterruptedException {
Thread t1 = new Thread(new CacheFiller(0));
Thread t2 = new Thread(new CacheFiller(1));
t1.start();
t2.start();
t1.join();
t2.join();
}
Why not just put that in a unit test?? None of this code is tested and a little
mistake could have far-reaching ramifications.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962 --- Comment #18 from Mehdi Houshmand med1...@gmail.com 2012-03-09 09:15:10 UTC --- snip Why not just put that in a unit test?? None of this code is tested and a little mistake could have far-reaching ramifications. By that I obviously meant the PropertyCache version of this test, however you tested your latest patch in development... -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962 Alex Giotis alex.gio...@gmail.com changed: What|Removed |Added Attachment #28412|0 |1 is obsolete|| --- Comment #19 from Alex Giotis alex.gio...@gmail.com 2012-03-09 16:04:06 UTC --- Created attachment 28447 -- https://issues.apache.org/bugzilla/attachment.cgi?id=28447 Patch update including a PropertyCacheTestCase * Updated patch against revision 1298724 of trunk * Resolved patch merge conflicts introduced by recent commits. * Updated code against the latest checkstyle 5.5 * Added a PropertyCacheTestCase -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962
Alexis Giotis alex.gio...@gmail.com changed:
What|Removed |Added
Attachment #27495|0 |1
is obsolete||
--- Comment #15 from Alexis Giotis alex.gio...@gmail.com 2012-03-02 16:21:57
UTC ---
Created attachment 28412
-- https://issues.apache.org/bugzilla/attachment.cgi?id=28412
Patch update to fix NPE on JDK5
Added a lock on the PropertyCache that prevents concurrent cleanup of the
cached objects.
There is no reason to concurrently cleanup the cache but most importantly it
protects from an NullPointerException occuring on Sun JDK5
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6312056
This issue was reported by Vincent. The updated patch does not include the
tests added by Mehdi for the following reasons:
- Make clear my contribution.
- There were some cleanups requested by Vincent that I did not fully understand
and I don't wish to delay it's processing.
- I don't think we get added value by testing the hashCode() equals()
implementations. The tests don't protect from future changes in the tested
classes (e.g. adding a new field) and don't protect if in the future another
class is added in the cache.
- To find broken hashCode() implementation of cached instances, the
PropertyCache counts the number of collisions and reports it in the log if it
exceeds a number.
Of course anyone is welcomed to add tests, if he wishes so. For completeness,
below is Vincent's test that reveals the JDK5 bug:
private final PropertyCacheInteger cache = new PropertyCacheInteger();
private class CacheFiller implements Runnable {
private final int start;
CacheFiller(int start) {
this.start = start;
}
public void run() {
for (int i = 0; i 100; i++) {
cache.fetch(new Integer(start + i));
}
}
}
public void testCleanUp() throws InterruptedException {
Thread t1 = new Thread(new CacheFiller(0));
Thread t2 = new Thread(new CacheFiller(1));
t1.start();
t2.start();
t1.join();
t2.join();
}
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962 --- Comment #16 from Alex Giotis alex.gio...@gmail.com 2012-03-02 16:33:36 UTC --- *** Bug 51625 has been marked as a duplicate of this bug. *** -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962 --- Comment #14 from Alexis Giotis alex.gio...@gmail.com 2011-12-15 12:27:57 UTC --- Hi Medhi, Now that you are a commiter and mockito is used, is there a reason for not applying this patch ? -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962 --- Comment #13 from Alexis Giotis alex.gio...@gmail.com 2011-10-24 16:59:20 UTC --- Now that there was a vote and Mockito has been accepted, what about applying the patch ? Is there something holding it ? -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962 --- Comment #12 from Simon Pepping spepp...@apache.org 2011-09-27 08:25:35 UTC --- (In reply to comment #9) Some of the tests added require Mockito, I tried to avoid using mocking as much as possible for the obvious reason that the commiters haven't agreed to add it as a dependency. However, some of these classes were are a nightmare to test without mocking them. I have no problem with the addition of Mickito to FOP's dependencies, since it helps writing true unit tests. -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962 --- Comment #10 from Alexis Giotis alex.gio...@gmail.com 2011-09-15 12:09:38 UTC --- Medhi, thanks for checking fixing checkstyle issues. Using the Property.eq() definitely makes the equals() methods easier to read. I used the eclipse auto generated equals() methods to avoid making any mistake because I did not add the unit tests that you did. The only line I would delete is a // TODO Auto-generated method stub that is found in NCnamePropertyTestCase.java I guess the issues related to the usage of Mockito and the etiquette for replacing older work, are not for me to comment. I can only say that I checked your changes over my patch and I find them very good. I have also applied my previous patch on production and it works fine. -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962 --- Comment #11 from Mehdi Houshmand med1...@gmail.com 2011-09-15 13:01:18 UTC --- Excellent, thanks for looking over it, I agree, that //TODO shouldn't have gotten through. Thanks again for taking the time to check through it. -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962 Mehdi Houshmand med1...@gmail.com changed: What|Removed |Added Attachment #27477|0 |1 is obsolete|| --- Comment #9 from Mehdi Houshmand med1...@gmail.com 2011-09-14 15:22:44 UTC --- Created attachment 27495 -- https://issues.apache.org/bugzilla/attachment.cgi?id=27495 propery cache patch I've made a few changes to this patch a little: - Created a static method Property.eq() which tests for object equality (and reference equality for performance reasons) - Fixed some checkstyle issues, these were not specific to this patch but rather fixed some issues as I went along - Unit tests have been added Some of the tests added require Mockito, I tried to avoid using mocking as much as possible for the obvious reason that the commiters haven't agreed to add it as a dependency. However, some of these classes were are a nightmare to test without mocking them. This patch makes the previous work obsolete, I don't know what the etiquette is when making someone else's work obsolete. I mean no offence. -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962 Vincent Hennebert vhenneb...@gmail.com changed: What|Removed |Added Summary|Deadlock in PropertyCache |[PATCH] Deadlock in |class |PropertyCache class -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 46962] [PATCH] Deadlock in PropertyCache class
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962 Alexis Giotis alex.gio...@gmail.com changed: What|Removed |Added Attachment #27358|0 |1 is obsolete|| --- Comment #8 from Alexis Giotis alex.gio...@gmail.com 2011-09-09 16:25:09 UTC --- Created attachment 27477 -- https://issues.apache.org/bugzilla/attachment.cgi?id=27477 Updated patch with rewritten PropertyCache that fixes more hashCode/equals() problems -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
