Bugfix - Fix race condition in the redux library

[OrlaM]

📜 Tickets

Jira ticket
Github issue

💡 Description

While helping Roux debug an issue with his redux code he uncovered a race condition in redux itself. This PR should address that race condition.

The issue was that if multiple actions are dispatched during the process of reducing for an ongoing action then those actions can be dispatched out of order.

This fix removes the actionRunning check because it's not working as expected and instead dispatches each action async on the main thread. Since all actions are executed on the main thread this means that actions will always be executed sequentially and in the correct order.

📝 Checklist

You have to check all boxes before merging

• Filled in the above information (tickets numbers and description of your work)
• Updated the PR name to follow our PR naming guidelines
• Wrote unit tests and/or ensured the tests suite is passing
• When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
• If needed, I updated documentation / comments for complex code and public methods
• If needed, added a backport comment (example @Mergifyio backport release/v120)

[mattreaganmozilla]

@OrlaM @adudenamedruby IIRC this is what we had implemented previously, and there were scenarios in which dispatch_async'ing for every action also created ordering problems. I can try to find the specific bug(s), but it was a while ago and I'd need to do some digging.

I'm a bit concerned this may be a case of whack-a-mole where the order is what we expect in some situations and not for others, and vice versa depending on how they're dispatch-async'd by the Store.

Sidenote*: I think it would be useful for us to create some unit tests for our Redux store that explicitly define (and enforce) the expected execution order for various dispatch scenarios. They would need to be async tests but we have very similar tests (almost the exact same thing) implemented in EventQueueTests.swift and they've been reliable on CI.

[mattreaganmozilla]

@OrlaM @adudenamedruby This is going to create some problems of its own:

// foo = false
store.dispatch(.toggleFoo)
// Code expects now expects foo == true
doSomethingBasedOnFooBeingTrue() // Problems

In pure-Redux land where everything gets routed through Redux (and thus everything is dispatch_async'd) it would be less of a concern, but right now we definitely have code in a variety of places where things work similarly to the above (though much more complicated, but the fundamental issue is the same). I'm fairly certain this was the problem we had before when we had been dispatch_async'ing everything.

[cyndichin]

create some unit tests for our Redux store that explicitly define (and enforce) the expected execution order for various dispatch scenarios

@mattreaganmozilla happy to create a ticket and look into this if useful

@adudenamedruby curious about the issue that discovered this, do you have test steps or description of the workflow that led to the discovery?