Odonno / ReduxSimple

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

Synchronous Effect leads to strange action ordering #87

Closed akaegi closed 3 years ago

akaegi commented 3 years ago

Strange situation with these two effects:

  1. GetIncRandomActionEffect that dispatches action GetIncRandomFulfilledAction with random number on observing GetIncRandomAction
  2. TrackActionEffect that just Console.WriteLine's the action

When ordering effects like this

store.RegisterEffects(
                TrackActionEffect(store),
                GetIncRandomActionEffect(store));

output is ok:

ReduxSimple action observed: ConsoleApp1.IncRandomAction
ReduxSimple action observed: ConsoleApp1.IncRandomFulfilledAction

Changing the order of the effects, changes the order of the reported actions!!

store.RegisterEffects(
             GetIncRandomActionEffect(store),
             TrackActionEffect(store));

output is inverted:

ReduxSimple action observed: ConsoleApp1.IncRandomFulfilledAction
ReduxSimple action observed: ConsoleApp1.IncRandomAction

Must have something to do with the underlying Subject :-| Or have I misunderstood something conceptually? My naïve view was that doing "synchronous" effects is legitimate...

Full example code: Program.cs.txt

Odonno commented 3 years ago

Hi @akaegi

Thank you for your reproduction code BTW. It really helps to understand your problem. From what I see, it is indeed synchronous and it should work like expected : action 1 => action 2 and not the other way.

It's not something I see everyday but I will try to fix this. My first assumption would be that the 2 effects run on 2 different threads. I am not sure of that but if it's true I think that synchronizing the underlying Observable should be the solution.

I also note your interesting idea of passing the Store in the CreateEffect function.

akaegi commented 3 years ago

Hi David, I'll also investigate on this a little bit. Quite interesting. I don't think it has anything to do with multithreading. AFAIK reactive-stuff runs single-threaded by default. I rather suspect that this behavior stems from the usage of the Subject as-is. I'll try to reproduce the observed outcome with a simple Subject example in itself. If it has the same outcome, than maybe the reactive guys can give some help on this.

akaegi commented 3 years ago

Quite as I expected: Subject-only example gives the same result:

var s = new Subject<string>();

s.Subscribe(msg =>
{
    if (msg == "IncRandomAction") 
        s.OnNext("IncRandomFulfilledAction");
});

s.Subscribe(msg =>
{
    Console.WriteLine(msg);
});

s.OnNext("IncRandomAction");

outputs:

IncRandomFulfilledAction
IncRandomAction

Now we have to find out if this is the expected semantics from a Subject... My understanding was that every Subscriber should get the same order of events...

Odonno commented 3 years ago

Hi David, I'll also investigate on this a little bit. Quite interesting. I don't think it has anything to do with multithreading. AFAIK reactive-stuff runs single-threaded by default. I rather suspect that this behavior stems from the usage of the Subject as-is. I'll try to reproduce the observed outcome with a simple Subject example in itself. If it has the same outcome, than maybe the reactive guys can give some help on this.

Well, no. I don't know the very detail how multithreading is handled with Rx.NET but it is. If you want to put everything in a single thread and avoid concurrency issues (like ordering), you should use the Synchronize method.

You can try this method if you like and see if it changes anything. But again, it can be that or it can be something else. I will investigate.

akaegi commented 3 years ago

Acc. to http://reactivex.io/documentation/contract.html:

There is no general guarantee that two observers of the same Observable will see the same sequence of items.

This is not an issue of multi-threading but of Subject semantics.

Subject notifies all subscribers OnNext (e1) in a loop but sequentially -> first subscriber notified (e1) -> issues another event e2 OnNext (this is pushed on the call stack) -> both subscribers notified for e2 -> Code returns to loop OnNext (e1) -> 2nd subscriber notified e1

If I replace Subject by ReplaySubject in my example, everything works as expected.

Odonno commented 3 years ago

Well, I understand the current behavior and how each element interact but it is not very clear to me where the problem is. I suspect that it can be because each effect run in a separate Observable, meaning they are not synchronized.

Odonno commented 3 years ago

Merging all effect sources does not fix the problem. :(

Odonno commented 3 years ago

Oh. Using a ConnectableObservable seems to fix the issue. Related article: https://michaelscodingspot.com/c-job-queues-with-reactive-extensions-and-channels/

From this article, I understand that all values of the Observable must be placed in the same scheduler (Scheduler.Default). And so when we subscribe to the ConnectableObservable, every action is now subscribed one at a time, one after the other.

Odonno commented 3 years ago

@akaegi You will now see the v3.6.0 in NuGet. Can you tell me if it fixes your issue?

akaegi commented 3 years ago

Thank you for your patience David with fixing this (I think) important bug!

Your bugfix looks fine to me and solves the bug. However by using Scheduler.Default multi-threading is introduced.

I'll submit asap a PR that lets the user pass another scheduler to the ReduxStore constructor.

Then either DispatcherScheduler.Current (WPF only, I don't know?) or new SynchronizationContextScheduler(SynchronizationContext.Current) can be passed and the actions are dispatched as usual on the "main thread". Still by dispatching the actions on the dispatcher queue, the order is correct, as demonstrated in a newly added test (EffectsActionOrderTest)