From d7c012930d2e0b6babf79cc7cb68d885e08a8672 Mon Sep 17 00:00:00 2001 From: akarnokd Date: Fri, 8 Jun 2018 12:41:19 +0200 Subject: [PATCH 1/4] 4.x: Fix timed delay hangs with dotCover --- .../ConcurrencyAbstractionLayerImpl.cs | 16 ++++------------ .../Tests/Linq/Observable/DelayTest.cs | 9 +++++++++ 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/Rx.NET/Source/src/System.Reactive/Concurrency/ConcurrencyAbstractionLayerImpl.cs b/Rx.NET/Source/src/System.Reactive/Concurrency/ConcurrencyAbstractionLayerImpl.cs index 2c1803bbe8..bdad63e8be 100644 --- a/Rx.NET/Source/src/System.Reactive/Concurrency/ConcurrencyAbstractionLayerImpl.cs +++ b/Rx.NET/Source/src/System.Reactive/Concurrency/ConcurrencyAbstractionLayerImpl.cs @@ -154,7 +154,7 @@ private sealed class Timer : IDisposable { private object _state; private Action _action; - private volatile System.Threading.Timer _timer; + private IDisposable _timer; public Timer(Action action, object state, TimeSpan dueTime) { @@ -169,7 +169,7 @@ public Timer(Action action, object state, TimeSpan dueTime) // Rooting of the timer happens through the Timer's state // which is the current instance and has a field to store the Timer instance. // - _timer = new System.Threading.Timer(_ => Tick(_), this, dueTime, TimeSpan.FromMilliseconds(System.Threading.Timeout.Infinite)); + Disposable.SetSingle(ref _timer, new System.Threading.Timer(_ => Tick(_), this, dueTime, TimeSpan.FromMilliseconds(System.Threading.Timeout.Infinite))); } } @@ -183,23 +183,15 @@ private static void Tick(object state) } finally { - SpinWait.SpinUntil(timer.IsTimerAssigned); - timer.Dispose(); + Disposable.TryDispose(ref timer._timer); } } - private bool IsTimerAssigned() => _timer != null; - public void Dispose() { - var timer = _timer; - if (timer != TimerStubs.Never) + if (Disposable.TryDispose(ref _timer)) { - _action = Stubs.Ignore; - _timer = TimerStubs.Never; _state = null; - - timer.Dispose(); } } } diff --git a/Rx.NET/Source/tests/Tests.System.Reactive/Tests/Linq/Observable/DelayTest.cs b/Rx.NET/Source/tests/Tests.System.Reactive/Tests/Linq/Observable/DelayTest.cs index 0c734f2e41..afba268557 100644 --- a/Rx.NET/Source/tests/Tests.System.Reactive/Tests/Linq/Observable/DelayTest.cs +++ b/Rx.NET/Source/tests/Tests.System.Reactive/Tests/Linq/Observable/DelayTest.cs @@ -704,6 +704,15 @@ public void Delay_TimeSpan_DefaultScheduler() Assert.True(Observable.Return(1).Delay(TimeSpan.FromMilliseconds(1)).ToEnumerable().SequenceEqual(new[] { 1 })); } + [Fact] + public void Task_Loop() + { + for (int i = 0; i < 1000; i++) + { + Observable.Return(1).Delay(TimeSpan.FromMilliseconds(1), DefaultScheduler.Instance).Wait(); + } + } + [Fact] public void Delay_DateTimeOffset_DefaultScheduler() { From cf40a12c6de0517b0794f96a97843522d0c9d630 Mon Sep 17 00:00:00 2001 From: akarnokd Date: Fri, 8 Jun 2018 14:22:07 +0200 Subject: [PATCH 2/4] Fix timer management in CALayer, less time in VirtualSchedulerTest --- .../ConcurrencyAbstractionLayerImpl.cs | 11 +------- .../Tests/Concurrency/VirtualSchedulerTest.cs | 27 +++++++++++++++---- .../Tests/Linq/Observable/DelayTest.cs | 9 ------- 3 files changed, 23 insertions(+), 24 deletions(-) diff --git a/Rx.NET/Source/src/System.Reactive/Concurrency/ConcurrencyAbstractionLayerImpl.cs b/Rx.NET/Source/src/System.Reactive/Concurrency/ConcurrencyAbstractionLayerImpl.cs index bdad63e8be..f59d32f954 100644 --- a/Rx.NET/Source/src/System.Reactive/Concurrency/ConcurrencyAbstractionLayerImpl.cs +++ b/Rx.NET/Source/src/System.Reactive/Concurrency/ConcurrencyAbstractionLayerImpl.cs @@ -161,16 +161,7 @@ public Timer(Action action, object state, TimeSpan dueTime) _state = state; _action = action; - // Don't want the spin wait in Tick to get stuck if this thread gets aborted. - try { } - finally - { - // - // Rooting of the timer happens through the Timer's state - // which is the current instance and has a field to store the Timer instance. - // - Disposable.SetSingle(ref _timer, new System.Threading.Timer(_ => Tick(_), this, dueTime, TimeSpan.FromMilliseconds(System.Threading.Timeout.Infinite))); - } + Disposable.SetSingle(ref _timer, new System.Threading.Timer(_ => Tick(_), this, dueTime, TimeSpan.FromMilliseconds(System.Threading.Timeout.Infinite))); } private static void Tick(object state) diff --git a/Rx.NET/Source/tests/Tests.System.Reactive/Tests/Concurrency/VirtualSchedulerTest.cs b/Rx.NET/Source/tests/Tests.System.Reactive/Tests/Concurrency/VirtualSchedulerTest.cs index 4349cf5075..a467890bcc 100644 --- a/Rx.NET/Source/tests/Tests.System.Reactive/Tests/Concurrency/VirtualSchedulerTest.cs +++ b/Rx.NET/Source/tests/Tests.System.Reactive/Tests/Concurrency/VirtualSchedulerTest.cs @@ -136,20 +136,36 @@ public void Virtual_ThreadSafety() { var scheduler = new TestScheduler(); var seq = Observable.Never(); + var d = default(IDisposable); - Task.Run(()=> + var sync = 2; + + Task.Run(() => { - Task.Delay(50).Wait(); - seq.Timeout(TimeSpan.FromSeconds(10), scheduler).Subscribe(s => { }); + if (Interlocked.Decrement(ref sync) != 0) + { + while (Volatile.Read(ref sync) != 0) ; + } + + Task.Delay(10).Wait(); + + d = seq.Timeout(TimeSpan.FromSeconds(5), scheduler).Subscribe(s => { }); }); var watch = scheduler.StartStopwatch(); try { - while (watch.Elapsed < TimeSpan.FromSeconds(20)) + if (Interlocked.Decrement(ref sync) != 0) { - scheduler.AdvanceBy(10); + while (Volatile.Read(ref sync) != 0) ; } + + while (watch.Elapsed < TimeSpan.FromSeconds(11)) + { + scheduler.AdvanceBy(50); + } + + throw new Exception("Should have thrown!"); } catch (TimeoutException) { @@ -158,6 +174,7 @@ public void Virtual_ThreadSafety() { Assert.True(false, string.Format("Virtual time {0}, exception {1}", watch.Elapsed, ex)); } + d?.Dispose(); } } } diff --git a/Rx.NET/Source/tests/Tests.System.Reactive/Tests/Linq/Observable/DelayTest.cs b/Rx.NET/Source/tests/Tests.System.Reactive/Tests/Linq/Observable/DelayTest.cs index afba268557..0c734f2e41 100644 --- a/Rx.NET/Source/tests/Tests.System.Reactive/Tests/Linq/Observable/DelayTest.cs +++ b/Rx.NET/Source/tests/Tests.System.Reactive/Tests/Linq/Observable/DelayTest.cs @@ -704,15 +704,6 @@ public void Delay_TimeSpan_DefaultScheduler() Assert.True(Observable.Return(1).Delay(TimeSpan.FromMilliseconds(1)).ToEnumerable().SequenceEqual(new[] { 1 })); } - [Fact] - public void Task_Loop() - { - for (int i = 0; i < 1000; i++) - { - Observable.Return(1).Delay(TimeSpan.FromMilliseconds(1), DefaultScheduler.Instance).Wait(); - } - } - [Fact] public void Delay_DateTimeOffset_DefaultScheduler() { From ee65e20116cf02837f72b29dd708b9a4d0ef17e7 Mon Sep 17 00:00:00 2001 From: akarnokd Date: Fri, 8 Jun 2018 14:31:30 +0200 Subject: [PATCH 3/4] Clear action as well. --- .../Concurrency/ConcurrencyAbstractionLayerImpl.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/Rx.NET/Source/src/System.Reactive/Concurrency/ConcurrencyAbstractionLayerImpl.cs b/Rx.NET/Source/src/System.Reactive/Concurrency/ConcurrencyAbstractionLayerImpl.cs index f59d32f954..92e661a601 100644 --- a/Rx.NET/Source/src/System.Reactive/Concurrency/ConcurrencyAbstractionLayerImpl.cs +++ b/Rx.NET/Source/src/System.Reactive/Concurrency/ConcurrencyAbstractionLayerImpl.cs @@ -182,6 +182,7 @@ public void Dispose() { if (Disposable.TryDispose(ref _timer)) { + _action = Stubs.Ignore; _state = null; } } From 65409293eac68a2e07187435ba273b2db0c8a0d2 Mon Sep 17 00:00:00 2001 From: akarnokd Date: Fri, 8 Jun 2018 17:31:01 +0200 Subject: [PATCH 4/4] Use thread-safe state cleanup --- .../Concurrency/ConcurrencyAbstractionLayerImpl.cs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/Rx.NET/Source/src/System.Reactive/Concurrency/ConcurrencyAbstractionLayerImpl.cs b/Rx.NET/Source/src/System.Reactive/Concurrency/ConcurrencyAbstractionLayerImpl.cs index 92e661a601..75c8d2de77 100644 --- a/Rx.NET/Source/src/System.Reactive/Concurrency/ConcurrencyAbstractionLayerImpl.cs +++ b/Rx.NET/Source/src/System.Reactive/Concurrency/ConcurrencyAbstractionLayerImpl.cs @@ -152,10 +152,12 @@ public void StartThread(Action action, object state) private sealed class Timer : IDisposable { - private object _state; + private volatile object _state; private Action _action; private IDisposable _timer; + private static readonly object DisposedState = new object(); + public Timer(Action action, object state, TimeSpan dueTime) { _state = state; @@ -170,7 +172,11 @@ private static void Tick(object state) try { - timer._action(timer._state); + var timerState = timer._state; + if (timerState != DisposedState) + { + timer._action(timerState); + } } finally { @@ -183,7 +189,7 @@ public void Dispose() if (Disposable.TryDispose(ref _timer)) { _action = Stubs.Ignore; - _state = null; + _state = DisposedState; } } }