Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prepare for disappearance of withSyncDispatch #112

Closed
TysonMN opened this issue Sep 11, 2019 · 5 comments
Closed

Prepare for disappearance of withSyncDispatch #112

TysonMN opened this issue Sep 11, 2019 · 5 comments

Comments

@TysonMN
Copy link
Member

TysonMN commented Sep 11, 2019

Elmish.WPF currenting uses Elmish.Program.withSyncDispatch here

|> Program.withSyncDispatch uiDispatch

As I understand it, three goals are achieved with this code.

  1. dispatch calls setState, which mutates UI state. That mutation must occur on the UI thread (otherwise an InvalidOperationException will occur).
  2. dispatch is not thread safe due to race conditions related to improper message processing. Executing dispatch only on the UI thread avoids this issue.
  3. dispatch (via setState) and the user (through the UI thread) can modify the state of the view. Even without using withSyncDispatch, WPF would synchronously call dispatch, which avoids "many" race conditions. However, it is also possible to call dispatch on a different thread (such as within an asynchronous command), and this can cause a race condition as describe in detail by @cmeeren in Discussion: MVU architecture gives feedbak loop when UpdateSourceTrigger=PropertyChanged and update loop is sufficiently slow #40 (comment). By executing dispatch on the UI thread, the mutation to the model reference by dispatch and the mutation to the view state by the UI thread stay consistent.

Are there any more goals that I missed?

The third one is the most complicated, so please do correct me if I said anything wrong.

In elmish/elmish#185 (comment), @et1975 says that withSyncDispatch will be removed in the next major release of Elmish.

How will Elmish.WPF be modified to handle this change?

@cmeeren
Copy link
Member

cmeeren commented Sep 11, 2019

Re. 3: I don't see async being part of the problem. IIRC the problem was that when the message loop (including update) did not run on the UI thread, there was a race condition between Elmish and the user in modifying the UI state. Running the loop on the UI thread blocks the UI, which solves the problem.

How will Elmish.WPF be modified to handle this change?

At a high level, @et1975 can probably answer this better than I can. Hopefully fairly trivially, like with the change to withSyncDispatch.

@TysonMN
Copy link
Member Author

TysonMN commented Sep 11, 2019

I don't see async being part of the problem. IIRC the problem was that when the message loop (including update) did not run on the UI thread, there was a race condition between Elmish and the user in modifying the UI state. Running the loop on the UI thread blocks the UI, which solves the problem.

Yes, you are correct. I only meant to suggest asynchronous computations as a common example of how a user might introduce multithreading into their application, but I forgot to explicitly say the part about there being an additional thread. I edited that part of my comment. Hopefully it is clear now.

@TysonMN
Copy link
Member Author

TysonMN commented Feb 4, 2021

I think the solution might be to change

let uiDispatch (innerDispatch: Dispatch<'msg>) : Dispatch<'msg> =
fun msg -> element.Dispatcher.Invoke(fun () -> innerDispatch msg)
program
|> Program.withSetState setState
|> Program.withSyncDispatch uiDispatch
|> Program.run

to

let runElmish () =
  program
  |> Program.withSetState setState
  |> Program.run

element.Dispatcher.Invoke runElmish

If so, then we can make this change now.

@et1975
Copy link
Member

et1975 commented Feb 4, 2021

Synchronized/Serialized dispatch is not going away, but it's an implementation detail of the current dispatch loop. I'm removing it from the Program abstraction and taking it as the loop implementation argument - you still get to specify how the synchronization happens, but you do it by parametrizing the loop, not the Program.

@TysonMN
Copy link
Member Author

TysonMN commented Feb 19, 2021

Great. Then we have nothing to worry about.

Thanks @et1975 :)

@TysonMN TysonMN closed this as completed Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants