Fix System.Timers.Timer regression on ARM
authorAlexander Köplinger <alex.koeplinger@outlook.com>
Mon, 9 Mar 2015 16:36:44 +0000 (17:36 +0100)
committerMarek Safar <marek.safar@gmail.com>
Mon, 2 May 2016 22:08:01 +0000 (00:08 +0200)
commit9dc369e0944b2168a230814995c602a2257776c2
treeb85917345e2fc5c938340c268c8e62ce896c37ac
parentc1414877eba9af378972ad1628ed2985679f913b
Fix System.Timers.Timer regression on ARM

When setting Interval = double.MaxValue like in the TimerTest.Interval_TooHigh_ThrowOnEnabled and
TimerTest.Interval_TooHigh_Enabled_Throw Mono unit tests there was a regression on ARM in that
the ArgumentOutOfRangeException wasn't thrown as expected.

Turns out, on ARM the call to (int)Math.Ceiling(double.MaxValue) as used in UpdateTimer() and Enabled setter
doesn't produce -2147483648 (i.e. Int32.MinValue) like on x86/amd64 but rather 2147483647 (i.e. Int32.MaxValue),
which is perfectly valid as the cast overflows and the result is an undefined value.
This means that the value passed to the underlying System.Threading.Timer is not negative
and no ArgumentOutOfRangeException is thrown there.

The fix is to properly check for interval>Int32.MaxValue like is done already in the constructor.
I pulled that logic out and introduced a separate method CalculateRoundedInterval().

Note: The thrown exception changes from an ArgumentOutOfRangeException inside of System.Threading.Timer
to an ArgumentException inside of CalculateRoundedInterval().
I think this has minimal impact as no-one should rely on this exception anyway and it actually matches
what MSDN (https://msdn.microsoft.com/en-us/library/system.timers.timer.interval(v=vs.110).aspx)
says should happen in this case (even though the MS.NET 4.5 implementation doesn't comply).
Mono's implementation also did the same before: https://github.com/mono/mono/blob/4cb3f77b4bbf703b1cda59db2f5aee206e35d31a/mcs/class/System/System.Timers/Timer.cs#L95-L96
mcs/class/referencesource/System/services/timers/system/timers/Timer.cs