Review Board 1.7.22

QPID-3222: Fix ttl overflow

Review Request #627 - Created April 20, 2011 and submitted

Gordon Sim
aconway, astitcher, chug, shuston
Fixes ttl overflow on the broker. Added equality operator for qpid::messaging::Duration (needed it in test and seemed generally valuable).
New test added, make check passes.
Ship it!
Posted (April 20, 2011, 4:59 p.m.)
The code looks OK, but do we really need to worry about people setting a TTL of more than 17 billion years? 
  1. Actually looking at the BZ, the real issue is giving FOREVER the correct special treatment at each step, it's not really an overflow issue at all.
  2. FOREVER is indeed the most important case. However as the value on the wire is uint64 (and is in ms), we need to make sure that no positive value that is sent is turned into a negative value and thus immediately marks the messages as expired. It is indeed very unlikely that anyone would have a real need to use anything other than FOREVER that would cause this, but other values could trigger the same thing, so why not fix the overflow entirely.
Posted (April 20, 2011, 6:04 p.m.)
1. To be complete the equality tests must propagate to the .NET binding as well. See patch below.

2. This patch changes the API/ABI a little does it not? 
   For the .NET case assume you have Duration A(100) and Duration B(100). 
   Before this patch A==B is false and after this patch A==B is true.
   How can we sell that?


Index: qpid/cpp/bindings/qpid/dotnet/src/Duration.h
--- qpid/cpp/bindings/qpid/dotnet/src/Duration.h	(revision 1089977)
+++ qpid/cpp/bindings/qpid/dotnet/src/Duration.h	(working copy)
@@ -81,8 +81,18 @@
             Duration ^ result = gcnew Duration(multiplier * dur->Milliseconds);
             return result;
-	};
+        static bool operator == (Duration ^ a, Duration ^ b)
+        {
+            return a->Milliseconds == b->Milliseconds;
+        }
+        static bool operator != (Duration ^ a, Duration ^ b)
+        {
+            return a->Milliseconds != b->Milliseconds;
+        }
     public ref class DurationConstants sealed
  1. As far as c++ API/ABI goes this is purely additive (it is a change but can't break anything). Prior to this change comparing two durations for equality would fail to compile due to the lack of equality operator.
    I would expect though that the semantic change from a .NET perspective would not only be acceptable but actually desired, no?
Ship it!
Posted (April 20, 2011, 7:25 p.m.)
Agreed. I'll patch the .NET file if you don't.