Error in PeriodType cache

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Error in PeriodType cache

Greg Inozemtsev
I noticed that the testForFields4 test method in TestPeriodType was
failing when running independently, so I decided to investigate.

The current cache implementation in PeriodType fails if the PeriodType
object with fields out-of-order is added first.  This is because the
object with fields out of order is added to the cache, but
PeriodType.equals expects the fields to be in order (as it is using
Arrays.equals).  The next object then falsely misses in the cache.
The fix is to cache a PeriodType object with fields in order.

This bug was not picked up by tests because the testForFields3 method
was running first and adding an object with the fields in the correct
order to the cache.  Therefore, I also made a change to testForFields4
to make it independent from testForFields3.

Thanks for the work on joda-time!

PS. Most of the credit goes to my wife (Laura Inozemtseva) for
catching the test case failure and troubleshooting.



diff -urp joda-time-2.0.orig/src/main/java/org/joda/time/PeriodType.java
joda-time-2.0/src/main/java/org/joda/time/PeriodType.java
--- joda-time-2.0.orig/src/main/java/org/joda/time/PeriodType.java 2011-12-06
01:56:23.406957436 -0500
+++ joda-time-2.0/src/main/java/org/joda/time/PeriodType.java 2011-12-06
06:01:15.725820494 -0500
@@ -569,10 +569,10 @@ public class PeriodType implements Seria
         PeriodType checkPartType = new PeriodType(null, type.iTypes, null);
         PeriodType checkedType = (PeriodType) cache.get(checkPartType);
         if (checkedType != null) {
-            cache.put(inPartType, checkedType);
+            cache.put(checkPartType, checkedType);
             return checkedType;
         }
-        cache.put(inPartType, type);
+        cache.put(checkPartType, type);
         return type;
     }

diff -urp joda-time-2.0.orig/src/test/java/org/joda/time/TestPeriodType.java
joda-time-2.0/src/test/java/org/joda/time/TestPeriodType.java
--- joda-time-2.0.orig/src/test/java/org/joda/time/TestPeriodType.java 2011-12-06
01:56:23.390957347 -0500
+++ joda-time-2.0/src/test/java/org/joda/time/TestPeriodType.java 2011-12-06
06:00:05.321471377 -0500
@@ -522,12 +522,12 @@ public class TestPeriodType extends Test

     public void testForFields4() throws Exception {
         DurationFieldType[] types = new DurationFieldType[] {
-            DurationFieldType.weeks(),
+            DurationFieldType.days(),
             DurationFieldType.months(),
         };
         DurationFieldType[] types2 = new DurationFieldType[] {
             DurationFieldType.months(),
-            DurationFieldType.weeks(),
+            DurationFieldType.days(),
         };
         PeriodType type = PeriodType.forFields(types);
         PeriodType type2 = PeriodType.forFields(types2);

------------------------------------------------------------------------------
Cloud Services Checklist: Pricing and Packaging Optimization
This white paper is intended to serve as a reference, checklist and point of
discussion for anyone considering optimizing the pricing and packaging model
of a cloud services business. Read Now!
http://www.accelacomm.com/jaw/sfnl/114/51491232/
_______________________________________________
Joda-interest mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/joda-interest
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Error in PeriodType cache

jodastephen
Pushed the fix, thanks for the spot!
Stephen


On 6 December 2011 11:20, Greg Inozemtsev <[hidden email]> wrote:

> I noticed that the testForFields4 test method in TestPeriodType was
> failing when running independently, so I decided to investigate.
>
> The current cache implementation in PeriodType fails if the PeriodType
> object with fields out-of-order is added first.  This is because the
> object with fields out of order is added to the cache, but
> PeriodType.equals expects the fields to be in order (as it is using
> Arrays.equals).  The next object then falsely misses in the cache.
> The fix is to cache a PeriodType object with fields in order.
>
> This bug was not picked up by tests because the testForFields3 method
> was running first and adding an object with the fields in the correct
> order to the cache.  Therefore, I also made a change to testForFields4
> to make it independent from testForFields3.
>
> Thanks for the work on joda-time!
>
> PS. Most of the credit goes to my wife (Laura Inozemtseva) for
> catching the test case failure and troubleshooting.
>
>
>
> diff -urp joda-time-2.0.orig/src/main/java/org/joda/time/PeriodType.java
> joda-time-2.0/src/main/java/org/joda/time/PeriodType.java
> --- joda-time-2.0.orig/src/main/java/org/joda/time/PeriodType.java      2011-12-06
> 01:56:23.406957436 -0500
> +++ joda-time-2.0/src/main/java/org/joda/time/PeriodType.java   2011-12-06
> 06:01:15.725820494 -0500
> @@ -569,10 +569,10 @@ public class PeriodType implements Seria
>         PeriodType checkPartType = new PeriodType(null, type.iTypes, null);
>         PeriodType checkedType = (PeriodType) cache.get(checkPartType);
>         if (checkedType != null) {
> -            cache.put(inPartType, checkedType);
> +            cache.put(checkPartType, checkedType);
>             return checkedType;
>         }
> -        cache.put(inPartType, type);
> +        cache.put(checkPartType, type);
>         return type;
>     }
>
> diff -urp joda-time-2.0.orig/src/test/java/org/joda/time/TestPeriodType.java
> joda-time-2.0/src/test/java/org/joda/time/TestPeriodType.java
> --- joda-time-2.0.orig/src/test/java/org/joda/time/TestPeriodType.java  2011-12-06
> 01:56:23.390957347 -0500
> +++ joda-time-2.0/src/test/java/org/joda/time/TestPeriodType.java       2011-12-06
> 06:00:05.321471377 -0500
> @@ -522,12 +522,12 @@ public class TestPeriodType extends Test
>
>     public void testForFields4() throws Exception {
>         DurationFieldType[] types = new DurationFieldType[] {
> -            DurationFieldType.weeks(),
> +            DurationFieldType.days(),
>             DurationFieldType.months(),
>         };
>         DurationFieldType[] types2 = new DurationFieldType[] {
>             DurationFieldType.months(),
> -            DurationFieldType.weeks(),
> +            DurationFieldType.days(),
>         };
>         PeriodType type = PeriodType.forFields(types);
>         PeriodType type2 = PeriodType.forFields(types2);
>
> ------------------------------------------------------------------------------
> Cloud Services Checklist: Pricing and Packaging Optimization
> This white paper is intended to serve as a reference, checklist and point of
> discussion for anyone considering optimizing the pricing and packaging model
> of a cloud services business. Read Now!
> http://www.accelacomm.com/jaw/sfnl/114/51491232/
> _______________________________________________
> Joda-interest mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/joda-interest

------------------------------------------------------------------------------
Cloud Services Checklist: Pricing and Packaging Optimization
This white paper is intended to serve as a reference, checklist and point of
discussion for anyone considering optimizing the pricing and packaging model
of a cloud services business. Read Now!
http://www.accelacomm.com/jaw/sfnl/114/51491232/
_______________________________________________
Joda-interest mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/joda-interest
Loading...