Consider removing same-thread-enforcement from getState

[patjackson52]

Reading the state is often needed from multiple threads. Consideration of removing, or making optional, the same-thread-enforcement from the getState function to allow more flexibility in reading the state from the store. Calling from another thread will cause the check for isDispatching to fail if accessed while an action is dispatching. A solution that is needed that accounts allowing access from another thread, and respects actions that are currently being dispatched.

Sample senario: Android webview javascript interface calls methods from a thread pool. A common usecase is to return a token from the state. If main thread is being used for the store, then use must write code to get the token from the main thread in a blocking manner.

[DarioAhdoot]

As someone who is adopting redux to (in part) tame a codebase whose thread management is extremely poor, I like the fact that the store is not thread safe. At the cost of some inconvenience (i.e. if you need state values from another thread, you are forced to have a cached value ahead of time), having a non-thread safe store means that you are forced to know what thread your code is executing on. It imposes real constraints that could be valuable, at least in the scenario I find myself in. I certainly can appreciate your use case though. Perhaps, as you suggest, make it optional? Just my .02

[patjackson52]

@Diarrhio Thanks for the input, really do appreciate it. Another approach would be to really make it thread-safe by using atomic fu. I'm hesitant though to add any overhead (performance, memory, lib size). I'm also doing some experimenting with coroutines and channels both in the lib and as store enhancers. Ideally the core redux library remains small as simple as possible. In the near term I may make getState unrestricted. Would that help in your case?

[DarioAhdoot]

I've actually wrapped the store in a wrapper class, allowing you to dispatch from a background thread (with the caveat that you must know that it will post to execute on the main thread) and which has a property called state but which asserts if you're not on the UI thread. So, unrestricting getState is fine with me as it won't actually change anything.

I've really been locking things down and putting up huge walls in our codebase, to prevent us from ever getting into the situation we've found ourselves in :)

[patjackson52]

@Diarrhio I'm testing out a wrapper as well. I think making same-thread-enforcement optional for those that want it and pointing to this wrapper approach will cover most use cases.

https://gist.github.com/patjackson52/744001821644161c6dbb8ae9ad352b92.js

[jason-whitted]

I'm definitely a fan of having the "current" state available across threads on an opt-in basis.

Maybe something like this?

fun getState(threadSafe: Boolean = true): State {
    if (threadSafe) {
        checkSameThread()
        check(!isDispatching) {
            """|You may not call store.getState() while the reducer is executing.
                |The reducer has already received the state as an argument.
                |Pass it down from the top reducer instead of reading it from the 
                |store.""".trimMargin()
        }
    }
    return currentState
}

This would provide backwards compatibility for store.getState() and store.state -- they would continue to provide the same single-thread requirement. A caller could opt-in with store.getState(threadSafe = false) to skip these checks.

[patjackson52]

I'm writing this up now and have a choice - include a SynchronizedStore in the library for those wanting to enable access from any thread, or leave it as a gist and people can copy it in as needed. It requires atomic-fu, so it brings in a dependency (small, but still want to avoid deps).

With multi thread access bugs will occur without synchronization. They will be intermittent and difficult to reproduce. This makes me think that including the SynchronizedStore and using when sameThreadEnforcement == false is the best approach. It keeps setup simple and one less thing.
Good minification/tree shaking should be able to remove it for those not using it and really concerned about size.

Any thoughts before I move forward?

[etalisoft]

@patjackson52 - I wrote kotlin-redux, kotlin-redux-thunk and kotlin-redux-reselect libraries based around coroutines. The thread syncrhonization is handled internally via a Mutex. We've been very pleased with it at this point as it significally reduced our code footprint not having to micromanage the thread. I also built observable support into the store via Kotlin's Channel and Flow.

I'm not going to spam your repositories with these libraries. If you are interested in them, let me know. I'm a huge proponent of OSS -- if this direction interests you hit me up, I'm more than willing to contribute.

[patjackson52]

@etalisoft Thanks for the input and suggestion. Using a mutex may indeed work. Before making the change to the lib I need to investigate mutex on all platforms (especially native). If you have any experience with it in multiplatform land sharing would be helpful.

Also want to look at perf of mutex vs synchronization.
Other consideration is adding coroutines vs atomicfu? which is bigger, which is more stable, more widely used. Gut feeling is coroutines will be used by most everyone so adding is not affecting most consumers. Also interested in adding channel/flow support to this library, so another + for coroutines.

Contributions are welcome!

[patjackson52]

Looking at putting mutex or synchronization into redux:

Pros: 1 - simple usage 2- eliminate thread problems

cons: 1 - lib size bloat - coroutines adds > 1mb (not sure after tree shaking/R8/etc. Probably not an issue, except potentially for JS only consumers

Alternative: Provide the SyncronizedStore(using either mutex or synchronization) as a separate dep. Make this the recommended approach and update docs. Use a createThreadSafeStore() & createStore() This will require a separate artifact. The code can live in the same repo in a thin module. New artifact can be:

dependencies {
    implementation "org.reduxkotlin:redux-kotlin-threadsafe:0.4.0"
}

Library developers (middleware, store-enhancers, etc) would only need the redux-kotlin dep, and they would be compatible with the createThreadSafeStore() due to same interface.

[patjackson52]

For more info look at docs: https://www.reduxkotlin.org/introduction/threading https://www.reduxkotlin.org/introduction/getting-started