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

stop subscriptions on window close, which is before dispatcher is disposed #355

Closed
wants to merge 1 commit into from

Conversation

TysonMN
Copy link
Member

@TysonMN TysonMN commented Mar 1, 2021

Fixes #353

The only changes are in the samples. The new code stops the infinite loops inside the subscriptions that would otherwise dispatch a message to a disposed Dispatcher.

Issue #353 becomes perfectly repeatable for me (in my experience) after decreasing the sleep time 1000 to 10. Do that as well as comment out the subscription to Window.OnClosing to confirm that the issue also becomes perfectly repeatable for you. Then comment that line back in to verify that this fix works.

@BentTranberg, I think this fixes #353. Do you agree?

@cmeeren
Copy link
Member

cmeeren commented Mar 1, 2021

Is this for merging, or just for confirming that #353 can be avoided in user code?

If it's for merging:

  • Is it possible to fix this in Elmish.WPF code instead of user code?
  • If not, I have some nitpicks here, but I won't spend time on that until I know this PR should be merged.

@BentTranberg
Copy link

BentTranberg commented Mar 1, 2021

Wow, there's lots of lessons for me in that code! It's the kind of magic that is beyond me unless the wizards care to share. Thanks a lot. Hasn't occurred to me that some "code behind" can be done on the F# side.

It is quite a mouthful for what seems to be such a simple issue, so if "possible to fix this in Elmish.WPF code instead" is potentially a much simpler solution in user code, that's certainly very interesting.

edit: I just looked at it briefly, but I'm off to an appointment right now, so can't run it now.
edit2: Tested it later in the day, and it's exactly as explained above.

@TysonMN
Copy link
Member Author

TysonMN commented Mar 1, 2021

This is for merging. 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.

@TysonMN
Copy link
Member Author

TysonMN commented Mar 1, 2021

Wow, there's lots of lessons for me in that code! It's the kind of magic that is beyond me unless the wizards care to share.

Indeed. There are a lot of ideas coming together here.

  1. We want to stop the (otherwise) infinite loop of dispatches, so I switched from Timer to an Async<_> because...
  2. Stopping some flow of control on other thread should be done with a CancellationToken, which can be obtained from a CancellationTokenSource. In particular, a nice property of CancellationTokenSource.Cancel is that it is idempotent, which means that calling it twice is equivalent to calling it once. It makes it easier to reason about impure code. We all know that the first "rule" of functional programming is to maximize the amount of pure code. The second "rule" could be, among the impure code, to maximize the amount of idempotent code. (Blog post in the queue on that topic.)
  3. Although the while loop looks infinite, the actual behavior is that the CancellationToken is checked as part of the predicate. I discovered this behavior in the process of writing this blog post.
  4. It is not enough to cancel the asynchronous computation. We also have to wait for the asynchronous computation to stop executing. Therefore, cancelling the CancellationToken is followed by waiting for the Task<_> that comes from a TaskCancellationSource<_>, which is set by the asynchronous computation when it is cancelled. It suffices to use SetResult there instead of TrySetResult, but I prefer TrySetResult because it is idempotent while SetResult is not (a second call to SetResult causes an exception to be thrown).

Did I miss anything? Is there any other parts that are unclear?

@BentTranberg
Copy link

magic that is beyond me

I realize now that it was an ambiguous statement - true in any case - and thanks a lot again. My original intention was meant to indicate I wouldn't have come up with that kind of solution, though CancellationToken was on my mind. I would not have thought of async, and I would have messed around in the C# project too. Now that I see it, I understand it, but your explanation is on a higher level - not something I could have expressed.

@cmeeren
Copy link
Member

cmeeren commented Mar 5, 2021

Sorry for taking so long. This (specifically, #353) is fixable in Elmish.WPF. See #358.

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

Successfully merging this pull request may close these issues.

TaskCanceledException on program end in timerTick in FileDialogsCmdMsg.Core
3 participants