diff --git a/Docs/releases.md b/Docs/releases.md index c419d281..c9459895 100644 --- a/Docs/releases.md +++ b/Docs/releases.md @@ -1,5 +1,8 @@ # Releases +## New in 5.8 +* Fixes potential for deadlock ([#407](https://github.com/mrpmorris/Fluxor/issues/407)) + ## New in 5.7 * Fixes memory leak when using `ActionSubscriber` or `SubscribeToAction` ([#378](https://github.com/mrpmorris/Fluxor/issues/378)) diff --git a/Source/Lib/Fluxor/Store.cs b/Source/Lib/Fluxor/Store.cs index 5620f4df..604182ef 100644 --- a/Source/Lib/Fluxor/Store.cs +++ b/Source/Lib/Fluxor/Store.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; @@ -20,7 +21,7 @@ public class Store : IStore, IActionSubscriber, IDisposable private readonly List Effects = new(); private readonly List Middlewares = new(); private readonly List ReversedMiddlewares = new(); - private readonly Queue QueuedActions = new(); + private readonly ConcurrentQueue QueuedActions = new(); private readonly TaskCompletionSource InitializedCompletionSource = new(); private readonly ActionSubscriber ActionSubscriber; @@ -144,32 +145,34 @@ void IDisposable.Dispose() private void ActionDispatched(object sender, ActionDispatchedEventArgs e) { - lock (SyncRoot) - { - // Do not allow task dispatching inside a middleware-change. - // These change cycles are for things like "jump to state" in Redux Dev Tools - // and should be short lived. - // We avoid dispatching inside a middleware change because we don't want UI events (like component Init) - // that trigger actions (such as fetching data from a server) to execute - if (IsInsideMiddlewareChange) - return; - - // If a dequeue is already in progress, we will just - // let this new action be added to the queue and then exit - // Note: This is to cater for the following scenario - // 1: An action is dispatched - // 2: An effect is triggered - // 3: The effect immediately dispatches a new action - // The Queue ensures it is processed after its triggering action has completed rather than immediately - QueuedActions.Enqueue(e.Action); - - // HasActivatedStore is set to true when the page finishes loading - // At which point DequeueActions will be called - if (!HasActivatedStore) - return; + // Do not allow task dispatching inside a middleware-change. + // These change cycles are for things like "jump to state" in Redux Dev Tools + // and should be short lived. + // We avoid dispatching inside a middleware change because we don't want UI events (like component Init) + // that trigger actions (such as fetching data from a server) to execute + if (IsInsideMiddlewareChange) + return; - DequeueActions(); - }; + + // This is a concurrent queue, so is safe even if dequeuing is already in progress + QueuedActions.Enqueue(e.Action); + + // HasActivatedStore is set to true when the page finishes loading + // At which point DequeueActions will be called + // So if it hasn't been activated yet, just exit and wait for that to happen + if (!HasActivatedStore) + return; + + // If a dequeue is still going then it will deal with the event we just + // queued, so we can exit at this point. + // This prevents a re-entrant deadlock + if (!IsDispatching) + { + lock (SyncRoot) + { + DequeueActions(); + }; + } } private void EndMiddlewareChange(IDisposable[] disposables) @@ -277,10 +280,8 @@ private void DequeueActions() IsDispatching = true; try { - while (QueuedActions.Count > 0) + while (QueuedActions.TryDequeue(out object nextActionToProcess)) { - object nextActionToProcess = QueuedActions.Dequeue(); - // Only process the action if no middleware vetos it if (Middlewares.All(x => x.MayDispatchAction(nextActionToProcess))) { @@ -300,7 +301,7 @@ private void DequeueActions() { IsDispatching = false; } - foreach(var dispatchedAction in dispatchedActions) + foreach (var dispatchedAction in dispatchedActions) TriggerEffects(dispatchedAction); } } diff --git a/Source/Tests/Fluxor.UnitTests/StoreTests/ThreadingTests/DispatchReentrancyTests/DispatchReentrancyTests.cs b/Source/Tests/Fluxor.UnitTests/StoreTests/ThreadingTests/DispatchReentrancyTests/DispatchReentrancyTests.cs new file mode 100644 index 00000000..07392fe0 --- /dev/null +++ b/Source/Tests/Fluxor.UnitTests/StoreTests/ThreadingTests/DispatchReentrancyTests/DispatchReentrancyTests.cs @@ -0,0 +1,53 @@ +using Fluxor.UnitTests.StoreTests.ThreadingTests.DispatchReentrancyTests.SupportFiles; +using System; +using System.Threading; +using System.Threading.Tasks; +using Xunit; + +namespace Fluxor.UnitTests.StoreTests.ThreadingTests.DispatchReentrancyTests +{ + public class DispatchReentrancyTests + { + private readonly IDispatcher Dispatcher; + private readonly IStore Subject; + private readonly IFeature Feature; + + [Fact] + public async Task WhenObserverSubscribesToAnAction_AndDispatchesAnActionFromANewThread_ThenThereShouldBeNoDeadlock() + { + Thread initialThread = Thread.CurrentThread; + Subject.SubscribeToAction(this, _ => + { + var thread = new Thread(() => + { + Thread.Sleep(50); + while (Thread.CurrentThread == initialThread) + Thread.Sleep(0); + + Dispatcher.Dispatch(new IncrementCounterAction()); + }); + thread.Start(); + thread.Join(); + }); + + var timeout = Task.Delay(1000); + var initialize = Task.Run(async () => + { + await Task.Yield(); + await Subject.InitializeAsync(); + }); + await Task.WhenAny(timeout, initialize); + Assert.False(timeout.IsCompleted, "Time out due to deadlock"); + } + + public DispatchReentrancyTests() + { + Dispatcher = new Dispatcher(); + Subject = new Store(Dispatcher); + + Feature = new CounterFeature(); + Subject.AddFeature(Feature); + } + + } +} diff --git a/Source/Tests/Fluxor.UnitTests/StoreTests/ThreadingTests/DispatchReentrancyTests/SupportFiles/CounterFeature.cs b/Source/Tests/Fluxor.UnitTests/StoreTests/ThreadingTests/DispatchReentrancyTests/SupportFiles/CounterFeature.cs new file mode 100644 index 00000000..d0498e94 --- /dev/null +++ b/Source/Tests/Fluxor.UnitTests/StoreTests/ThreadingTests/DispatchReentrancyTests/SupportFiles/CounterFeature.cs @@ -0,0 +1,8 @@ +namespace Fluxor.UnitTests.StoreTests.ThreadingTests.DispatchReentrancyTests.SupportFiles +{ + class CounterFeature : Feature + { + public override string GetName() => "Counter"; + protected override CounterState GetInitialState() => new(counter: 0); + } +} diff --git a/Source/Tests/Fluxor.UnitTests/StoreTests/ThreadingTests/DispatchReentrancyTests/SupportFiles/CounterState.cs b/Source/Tests/Fluxor.UnitTests/StoreTests/ThreadingTests/DispatchReentrancyTests/SupportFiles/CounterState.cs new file mode 100644 index 00000000..c05691d9 --- /dev/null +++ b/Source/Tests/Fluxor.UnitTests/StoreTests/ThreadingTests/DispatchReentrancyTests/SupportFiles/CounterState.cs @@ -0,0 +1,13 @@ +namespace Fluxor.UnitTests.StoreTests.ThreadingTests.DispatchReentrancyTests.SupportFiles +{ + public class CounterState + { + public readonly int Counter; + + public CounterState(int counter) + { + Counter = counter; + } + } +} + diff --git a/Source/Tests/Fluxor.UnitTests/StoreTests/ThreadingTests/DispatchReentrancyTests/SupportFiles/IncrementCounterAction.cs b/Source/Tests/Fluxor.UnitTests/StoreTests/ThreadingTests/DispatchReentrancyTests/SupportFiles/IncrementCounterAction.cs new file mode 100644 index 00000000..0d589009 --- /dev/null +++ b/Source/Tests/Fluxor.UnitTests/StoreTests/ThreadingTests/DispatchReentrancyTests/SupportFiles/IncrementCounterAction.cs @@ -0,0 +1,6 @@ +namespace Fluxor.UnitTests.StoreTests.ThreadingTests.DispatchReentrancyTests.SupportFiles +{ + class IncrementCounterAction + { + } +} diff --git a/Source/Tests/Fluxor.UnitTests/StoreTests/ThreadingTests/DispatchReentrancyTests/SupportFiles/IsolatedTests.cs b/Source/Tests/Fluxor.UnitTests/StoreTests/ThreadingTests/DispatchReentrancyTests/SupportFiles/IsolatedTests.cs new file mode 100644 index 00000000..692b67ef --- /dev/null +++ b/Source/Tests/Fluxor.UnitTests/StoreTests/ThreadingTests/DispatchReentrancyTests/SupportFiles/IsolatedTests.cs @@ -0,0 +1,6 @@ +namespace Fluxor.UnitTests.StoreTests.ThreadingTests.DispatchReentrancyTests.SupportFiles +{ + public class IsolatedTests : Middleware + { + } +}