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

Dispatch only if dispatcher is active #358

Closed
wants to merge 2 commits into from
Closed

Dispatch only if dispatcher is active #358

wants to merge 2 commits into from

Conversation

cmeeren
Copy link
Member

@cmeeren cmeeren commented Mar 5, 2021

Closes #355

Fixes #353
Fixes #330 (as of a0f75c1)

From #355 (comment):

I would also prefer a fix in Elmish.WPF instead. However, as @BentTranberg pointed out in #353 (comment), a fix in Elmish.WPF for #353 would probably also be a fix for #330, but as I said in #330 (comment), I don't have any more ideas for what a fix in Elmish.WPF could be.

I may be missing something, but I just dotted into Dispatcher and saw it had an HasShutdownStarted property, so I figured I could try guarding Invoke behind a check to that property. Seems to work like a charm.

The changes in ViewModel.fs are not needed to address #330 and #353, but for good measure I added this everywhere we call Dispatch.

Haven't looked into why this doesn't fix #330. Feel free to investigate.

Waiting for review and/or further discussion before merging.

@cmeeren
Copy link
Member Author

cmeeren commented Mar 5, 2021

This now also fixes #330.

In a0f75c1, I changed DispatchIfActive to always call Dispatch and just catch TaskCanceledException, instead of first checking HasShutdownStarted. This avoids a race condition where HasShutdownStarted changes between checking and invoking.

I can't immediately think of any good reasons not to do this, but let me know if this is a bad idea.

@TysonMN
Copy link
Member

TysonMN commented Mar 5, 2021

Oh, we were working on fixes at the same time! I just created PR #359 to fix #330.

I don't have time to look at your PR now, but I am eager to look at it later!

@TysonMN
Copy link
Member

TysonMN commented Mar 6, 2021

I may be missing something, but I just dotted into Dispatcher and saw it had an HasShutdownStarted property, so I figured I could try guarding Invoke behind a check to that property. Seems to work like a charm.
[...]
I changed DispatchIfActive to always call Dispatch and just catch TaskCanceledException, instead of first checking HasShutdownStarted. This avoids a race condition where HasShutdownStarted changes between checking and invoking.

I also considered guarding with HasShutdownStarted (as is listed in #330 (comment)) but reached the same conclusion as you.

I can't immediately think of any good reasons not to do this, but let me know if this is a bad idea.

I don't like it for two reasons.

  1. It pollutes the FirstChanceException event. I prefer the look-before-you-leap style over the ask-for-forgiveness-not permission style. I prefer for a thrown exception to mean that the code contains a bug.
  2. A thrown exception might cause the debugger to break (depending on the exception settings), which interrupts whatever the actual goal is. This can happen when people are exploring the samples or we test them but not when people depend on Elmish.WPF as a NuGet package.

I really like the fix I found for #300 that is in PR #359. Maybe the idea there of calling BeginInvoke can also be used to fix #353. I will look into that this weekend.

@cmeeren
Copy link
Member Author

cmeeren commented Mar 6, 2021

I also considered guarding with HasShutdownStarted (as is listed in #330 (comment))

I missed that, sorry. Didn't read that thread before coming across it myself.

  1. I prefer the look-before-you-leap style over the ask-for-forgiveness-not permission style

I agree, as long as there is a robust alternative. Evidently, HasShutdownStarted isn't robust enough. Eager to see what you find out regarding BeginInvoke.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants