Odonno / ReduxSimple

Simple Stupid Redux Store using Reactive Extensions
http://www.nuget.org/packages/ReduxSimple/
MIT License
144 stars 20 forks source link

Prevent Concurrent Dispatch #81

Closed RockNHawk closed 4 years ago

RockNHawk commented 4 years ago

I have tested store.Dispatch is not thread safe, must add a lock, here is the passed unit test with concurrent :

using Shouldly;
using Xunit;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Threading;
using System.Threading.Tasks;
using Converto;
using System.Reactive.Linq;

using static ReduxSimple.Tests.Setup.TodoListStore.Functions;
using static ReduxSimple.Reducers;
using static ReduxSimple.Effects;

namespace ReduxSimple.Tests
{
    using ConcurrentStore = ReduxSimple.ReduxStore<ConcurrentState>;

    public class ConcurrentState
    {
        public int NumberVal1 { get; set; }
        public List<string?> List1 { get; set; } = new List<string?>();
        public ImmutableList<string?> ImmutableArray1 { get; set; } = ImmutableList<string?>.Empty;
        public int EffectNumberVal { get; set; }

    }

    public class AddNumAction
    {
        public int Val { get; set; }
    }

    public class AddListAction
    {
        public string? Val { get; set; }
    }

    public class AddImmutableArrayAction
    {
        public string? Val { get; set; }
    }

    public class EffectAddNumAction
    {
        public int Val { get; set; }
    }

    public class ConcurrentTests
    {

        [Fact]
        public void CanDispatchConcurrent()
        {
            var initialState = new ConcurrentState() { };
            var store = new ConcurrentStore(
              CreateReducers(),
              initialState
          );

            int parallelCount = 1000;
            System.Threading.Tasks.Parallel.For(0, parallelCount, i =>
            {
                var s = i.ToString();
                lock (this)
                {
                    store.Dispatch(new AddNumAction { Val = 1, });
                }
                lock (this)
                {
                    store.Dispatch(new AddListAction { Val = s, });
                }
                lock (this)
                {
                    store.Dispatch(new AddImmutableArrayAction { Val = s, });
                }
            });

            store.State.EffectNumberVal.ShouldBe(0);
            store.State.NumberVal1.ShouldBe(parallelCount);
            store.State.List1.Count.ShouldBe(parallelCount);
            store.State.ImmutableArray1.Count.ShouldBe(parallelCount);
        }

        [Fact]
        public void CanDispatchConcurrentWithEffects()
        {
            var initialState = new ConcurrentState() { };
            var store = new ConcurrentStore(
              CreateReducers(),
              initialState
          );

            int effetcRunCount = 0;

            var effectWithDispatch = CreateEffect<ConcurrentState>(
              () => store.ObserveAction<AddNumAction>()
              .Do(_ => { Interlocked.Increment(ref effetcRunCount); })
              .Select(_ =>
                    {
                        store.Dispatch(new EffectAddNumAction { Val = 1, });
                        return new EffectAddNumAction { Val = 1 };
                        // return new object[] {
                        // new EffectAddNumAction{Val = 1},
                        // new EffectAddNumAction{Val = 1},
                        // };
                    }),
              true
          );

            store.RegisterEffects(
                effectWithDispatch
            );

            int parallelCount = 10000;
            System.Threading.Tasks.Parallel.For(0, parallelCount, i =>
            {
                var s = i.ToString();
                lock (this)
                {
                    store.Dispatch(new AddNumAction { Val = 1, });
                }
                lock (this)
                {
                    store.Dispatch(new AddListAction { Val = s, });
                }
                lock (this)
                {
                    store.Dispatch(new AddImmutableArrayAction { Val = s, });
                }
            });

            effetcRunCount.ShouldBe(parallelCount);
            store.State.EffectNumberVal.ShouldBe(parallelCount * 2);
            store.State.NumberVal1.ShouldBe(parallelCount);
            store.State.List1.Count.ShouldBe(parallelCount);
            store.State.ImmutableArray1.Count.ShouldBe(parallelCount);
        }

        public static IEnumerable<On<ConcurrentState>> CreateReducers()
        {
            return new List<On<ConcurrentState>>
            {
                On<AddNumAction, ConcurrentState>(HandleNum ),
                On<AddListAction, ConcurrentState>( HandleList),
                On<AddImmutableArrayAction, ConcurrentState>(HandleImmutableArray ),
                On<EffectAddNumAction, ConcurrentState>(HandleEffectNum ),
            };
        }

        private static ConcurrentState HandleNum(ConcurrentState state, AddNumAction action)
        {
            return state.With(
                           new
                           {
                               NumberVal1 = state.NumberVal1 + action.Val,
                           }
                       );
        }

        private static ConcurrentState HandleEffectNum(ConcurrentState state, EffectAddNumAction action)
        {
            return state.With(
                           new
                           {
                               EffectNumberVal = state.EffectNumberVal + action.Val,
                           }
                       );
        }

        private static ConcurrentState HandleList(ConcurrentState state, AddListAction action)
        {
            var list = new List<string?>(state.List1);
            list.Add(action.Val);
            return state.With(new
            {
                List1 = list,
            });
        }

        private static ConcurrentState HandleImmutableArray(ConcurrentState state, AddImmutableArrayAction action)
        {
            var arr = state.ImmutableArray1.Add(action.Val);
            return state.With(new
            {
                ImmutableArray1 = arr,
            });
        }

    }
}
Odonno commented 4 years ago

@RockNHawk Thank you for this very explicit presentation of the bug, I never thought you'd have concurrent issue using the library. I reused your test in some way and so I managed to prevent concurrent dispatch pretty easily.

The v3.5.1 of ReduxSimple is now available. Tell me what you think!

RockNHawk commented 4 years ago

@RockNHawk Thank you for this very explicit presentation of the bug, I never thought you'd have concurrent issue using the library. I reused your test in some way and so I managed to prevent concurrent dispatch pretty easily.

The v3.5.1 of ReduxSimple is now available. Tell me what you think!

Than you for your respond, my application is a little special and calls Store.Dispatch in parallel, so I need to lock it to make it work properly. I wonder if the new version directly prevents concurrent calls? In fact, there is a demand for concurrent calls. If the new version can provide a switch for concurrent detection, I can use the new version.

Odonno commented 4 years ago

No lock is necessary on your side. As from v3.5.1, you can concurrently dispatch actions and everything will be handled in the library. If you have any problem, feel free to report the bug to me. :)