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

Infinite loop when messages are sent too quickly in succession #161

Closed
TimLariviere opened this issue Aug 15, 2018 · 15 comments · Fixed by #237
Closed

Infinite loop when messages are sent too quickly in succession #161

TimLariviere opened this issue Aug 15, 2018 · 15 comments · Fixed by #237
Labels
t/bug Something isn't working as expected

Comments

@TimLariviere
Copy link
Member

TimLariviere commented Aug 15, 2018

Updated with workaround

Applies to: Android (maybe iOS)

Description:
To update the model along with what the user does, I listen to events like TextChanged.
This works fine.
But under some circumstances, such event might trigger an infinite update-view loop which will freeze the app.
Based on what I do and the logs, I suspect EXF doesn't like TextChanged to trigger too quickly in a row.
I will update this issue when I find more about this.

iOS seems not to be affected.

Logs
In ElmishContacts, I searched for a contact "Hoya Booya" and then erased my search.

Message: MainPageMsg (TabAllContactsMsg (UpdateFilterText "Ho"))
Updated model: ...
Create Xamarin.Forms.StackLayout
Create Xamarin.Forms.Label
View, model = ...
View result: NavigationPage(...)@821839065
Updating NavigationPage, prevCount = 1, newCount = 1
Adjust page number 0
Create Xamarin.Forms.StackLayout
Create Xamarin.Forms.Image
Create Xamarin.Forms.StackLayout
Create Xamarin.Forms.Label
Create Xamarin.Forms.Label
Create Xamarin.Forms.Image

Message: MainPageMsg (TabAllContactsMsg (UpdateFilterText "H"))
Updated model: ...
View, model = ...
View result: NavigationPage(...)@571913183
Updating NavigationPage, prevCount = 1, newCount = 1
Adjust page number 0
Create Xamarin.Forms.StackLayout
Create Xamarin.Forms.Label

Message: MainPageMsg (TabAllContactsMsg (UpdateFilterText ""))
Updated model: ...
View, model = ...
View result: NavigationPage(...)@936190227
Updating NavigationPage, prevCount = 1, newCount = 1
Adjust page number 0
Create Xamarin.Forms.StackLayout
Create Xamarin.Forms.Label
Create Xamarin.Forms.StackLayout
Create Xamarin.Forms.Image
Create Xamarin.Forms.StackLayout
Create Xamarin.Forms.Label
Create Xamarin.Forms.Label
[IInputConnectionWrapper] beginBatchEdit on inactive InputConnection
[IInputConnectionWrapper] getTextBeforeCursor on inactive InputConnection
Create Xamarin.Forms.Image

Message: MainPageMsg (TabAllContactsMsg (UpdateFilterText "H"))
Updated model: ...
View, model = ...
View result: NavigationPage(...)@906179063
Updating NavigationPage, prevCount = 1, newCount = 1
Adjust page number 0
Create Xamarin.Forms.StackLayout
Create Xamarin.Forms.Label

Message: MainPageMsg (TabAllContactsMsg (UpdateFilterText ""))
Updated model: ...
Create Xamarin.Forms.StackLayout
Create Xamarin.Forms.Label
View, model = ...
View result: NavigationPage(...)@765267125
Updating NavigationPage, prevCount = 1, newCount = 1
Adjust page number 0
Create Xamarin.Forms.StackLayout
Create Xamarin.Forms.Image
[IInputConnectionWrapper] getTextAfterCursor on inactive InputConnection
Create Xamarin.Forms.StackLayout
Create Xamarin.Forms.Label
Create Xamarin.Forms.Label
Create Xamarin.Forms.Image

Message: MainPageMsg (TabAllContactsMsg (UpdateFilterText "H"))
Updated model: ...
View, model = ...
View result: NavigationPage(...)@991281053
Create Xamarin.Forms.StackLayout
Create Xamarin.Forms.Label
Updating NavigationPage, prevCount = 1, newCount = 1
Adjust page number 0

Message: MainPageMsg (TabAllContactsMsg (UpdateFilterText ""))
Updated model: ...
View, model = ...
View result: NavigationPage(...)@52470701
Updating NavigationPage, prevCount = 1, newCount = 1
Adjust page number 0
Create Xamarin.Forms.StackLayout
Create Xamarin.Forms.Label
Create Xamarin.Forms.StackLayout
Create Xamarin.Forms.Image
Create Xamarin.Forms.StackLayout
Create Xamarin.Forms.Label
Create Xamarin.Forms.Label
[IInputConnectionWrapper] getSelectedText on inactive InputConnection
Create Xamarin.Forms.Image

Message: MainPageMsg (TabAllContactsMsg (UpdateFilterText "H"))
Updated model: ...
View, model = ...
View result: NavigationPage(...)@594339090
Updating NavigationPage, prevCount = 1, newCount = 1
Adjust page number 0
Create Xamarin.Forms.StackLayout
Create Xamarin.Forms.Label

Message: MainPageMsg (TabAllContactsMsg (UpdateFilterText ""))
Updated model: ...
View, model = ...
View result: NavigationPage(...)@1068286510
Updating NavigationPage, prevCount = 1, newCount = 1
Adjust page number 0
Create Xamarin.Forms.StackLayout
Create Xamarin.Forms.Label
[IInputConnectionWrapper] endBatchEdit on inactive InputConnection
Create Xamarin.Forms.StackLayout
Create Xamarin.Forms.Image
Create Xamarin.Forms.StackLayout
Create Xamarin.Forms.Label
Create Xamarin.Forms.Label
Create Xamarin.Forms.Image
[IInputConnectionWrapper] beginBatchEdit on inactive InputConnection
[IInputConnectionWrapper] getTextBeforeCursor on inactive InputConnection

// And on, and on...

Repro:
https://github.com/TimLariviere/EXF_ReproInfiniteLoop

  1. Start the repro
  2. Type some text slowly in the Entry field (like you would normally)
  3. Working!
  4. Start mashing your keyboard (write quickly) or erase the field
  5. Not working. EXF goes berserk.

Workaround:
The issue only occurs when the UI updates quicker than EXF follows, so one way to prevent that is to throttle the TextChanged event and only send a single message when no more TextChanged are triggered for a given time.

Here's an example with a 250ms timeout.

One important point: we need to "fix" the throttle function with fixf => fixf throttle dispatch 250.
The throttle func start a new loop each time it is called, and without caching it with fixf, a new loop would be created each time the view refreshes.

let throttle fn timeout =
    let mailbox = MailboxProcessor.Start(fun agent ->
        let rec loop lastMsg = async {
            let! r = agent.TryReceive(timeout)
            match r with
            | Some msg ->
                return! loop (Some msg)
            | None when lastMsg.IsSome ->
                fn lastMsg.Value
                return! loop None
            | None ->
                return! loop None
        }
        loop None
    )
    mailbox.Post

let view (model: Model) dispatch =
    let throttledDispatch = fixf throttle dispatch 250

    View.ContentPage(
      content = View.StackLayout(
        children = [
            View.Entry(
                text=model.TextWorkaround,
                textChanged=(fun e -> throttledDispatch (TextChangedWorkaround e.NewTextValue)))
        ]))
@willsam100
Copy link

I have also been able to reproduce this issue.

I would prefer to use the completed handler as this does not fire as often. However, as per the Xamarin Forms documentation, it only fires when the user presses the done button via the keyboard, missing the case when the user simply taps out of the cell.

Having an event that fires when focus is lost on the control could be another possible solution.

@TimLariviere
Copy link
Member Author

TimLariviere commented Aug 23, 2018

One way to explain this behavior is that EXF is too slow compared to Android firing the events, and can't keep in sync. When comes time to refresh the UI based on the model, EXF is too late and sets the wrong values.

Android TextChanged “a” (1)
Android TextChanged “ab” (2)
EXF update and view “a” (1) => Android triggered “ab -> a”
Android TextChanged “a” (3)
EXF update and view “ab” (2) => Android triggered “a -> ab”
Android TextChanged “ab” (4)
EXF update and view “a” (3) => Android triggered “ab -> a”
Android TextChanged “a” (5)
EXF update and view “ab” (4) => Android triggered “a -> ab”
Android TextChanged “ab” (6)
etc.

On iOS, in a general manner, things are executed more quickly, allowing EXF to follow closely what's happening on the UI. This might explain the fact I did not reproduce the bug on iOS.

One workaround could be a throttling mecanism that would wait a few milliseconds (for the TextChanged events to stop) before dispatching a message.

let view (model: Model) dispatch =
    View.ContentPage(
      content = View.StackLayout(padding = 20.0, verticalOptions = LayoutOptions.Center,
        children = [
            View.Entry(text=model.Text, textChanged=(fun e ->
                throttle dispatch (TextChanged e.NewTextValue))
            )
        ]))

@TimLariviere
Copy link
Member Author

Someone wrote an article on how to throttle the TextChanged event in F# on Xamarin.Forms
https://nbevans.wordpress.com/2014/08/09/a-simple-stereotypical-javascript-like-debounce-service-for-f/

@TimLariviere
Copy link
Member Author

I've updated the first comment with a workaround inspired by the article.

@dsyme
Copy link
Collaborator

dsyme commented Sep 5, 2018

@TimLariviere Do you think this should be de-bounced in EXF itself?

@dsyme dsyme added the t/bug Something isn't working as expected label Sep 5, 2018
@TimLariviere
Copy link
Member Author

@dsyme Yes, I think it's going to be a recurring scenario (might affect iOS too in some extreme cases)
But maybe only provide an on-demand debounce helper like fixf and dependsOn?
Not everyone wants to wait for the timeout to occur before reacting to the event.

Ideally something like that:

View.Entry(textChanged=(debounce 250 (fun e -> dispatch (TextChanged e))))

But this can be quite hard to implement:

  • How do we manage the life cycle of such a function? (no event on the control to check when it appears/disappears)
  • How to differentiate a debounced event handler from a normal one, when recycling/updating?
  • Are mailbox agents a good solution?

@kevinha
Copy link

kevinha commented Nov 2, 2018

@willsam100 since it is now possible to capture the control we can do this:

View.Entry
    (text = model.Name, 
     created = (fun entry -> 
         entry.Unfocused.Add(fun args -> 
             if model.Name <> entry.Text then dispatch (NameChanged entry.Text))))

Are there any downsides to this, apart from having to access the underlying control? Is it better not to compare the model and control values?

The Unfocused event has been around since at least XF3.1. Is there a particular reason it is not in Fabulous?

@TimLariviere
Copy link
Member Author

Are there any downsides to this, apart from having to access the underlying control? Is it better not to compare the model and control values?

Seems like a good solution. Comparing the model and the control is ok for me.
Though I don't really like to rely on Focus. I remember having troubles in WPF where focus was not lost when doing other things (clicking a button, ...)

The Unfocused event has been around since at least XF3.1. Is there a particular reason it is not in Fabulous?

This is one of the many forgotten properties/events that we're currently missing in Fabulous :)
If you want to, we'll gladly accept a PR that fixes that.

@kevinha
Copy link

kevinha commented Nov 4, 2018

I've tried using focus on a couple of Android phones and it works ok for our app. Not had the chance to try it out on iOS yet, emulator or hardware. On the other hand textChanged breaks very quickly on the Android phones but is not a problem on the emulator. The sooner there is an accepted solution for that the better, but that's currently beyond my knowledge.

I'll do a PR for the event as soon as I get the time.

@kevinha
Copy link

kevinha commented Nov 5, 2018

Further to the suggestion to use Unfocused, above, the model values are captured just once when the control is created which is not what was intended. I got round this by having a mutable module scope binding for the model which is captured by the created callback and is assigned to on each view() call. Is there a better way to do this?

@TimLariviere
Copy link
Member Author

TimLariviere commented Nov 5, 2018

Ah yes indeed.
I think the easiest way to solve this would be to extract the condition from the event handler.
Losing focus won't happen as often as TextChanged, so you should be fine dispatching NameFocusLost everytime.

let update msg model =
     match msg with
     | NameFocusLost text ->
         if model.Name <> text then
             { model with Name = text }, (Cmd.ofMsg NameChanged)
         else
             model, Cmd.none
     | NameChanged ->
        // Do something on name changed

let view model dispatch =
     View.Entry
         (text = model.Name, 
          created = (fun entry ->
              entry.Unfocused.Add(fun args -> dispatch (NameFocusLost entry.Text))))

@TimLariviere
Copy link
Member Author

TimLariviere commented Nov 6, 2018

Found another way to debounce that we could include in Fabulous.
Unlike the one with MailboxProcessor, this one isn't an infinite loop and doesn't need disposing.
Not sure if this is really performant, or if there's corner cases where it won't work.

let debounce<'T> =
    let memoizations = Dictionary<obj, CancellationTokenSource>(HashIdentity.Structural)

    fun (timeout: int) (fn: 'T -> unit) value ->
        let key = fn.GetType()

        // Cancel previous debouncer
        match memoizations.TryGetValue(key) with
        | true, cts -> cts.Cancel()
        | _ -> ()

        // Create a new cancellation token and memoize it
        let cts = new CancellationTokenSource()
        memoizations.[key] <- cts

        // Start a new debouncer
        (async {
            try
                // Wait timeout to see if another event will cancel this one
                do! Async.Sleep timeout

                // If still not cancelled, then proceed to invoke the callback and discard the unused token
                memoizations.Remove(key) |> ignore
                fn value
            with
            | _ -> ()
        })
        |> (fun task -> Async.StartImmediate(task, cts.Token))

We could use it like that

View.Entry(textChanged=(debounce 250 (fun e -> dispatch (TextChanged e))))

@Hardt-Coded
Copy link

Hi. The latestest version of the decounce doesn't work. I get this loop on the phone and in the emulator, when I type to fast. I didn't check the version with the agent, yet.

@TimLariviere
Copy link
Member Author

TimLariviere commented Nov 17, 2018

@DieselMeister I'm interested to know how you got it to break. Do you have an example or a repro?
I'm using it in ElmishContacts and it works, even with the ultra fast typing of Xamarin.UITest (tested on 16 android and 16 iOS devices on Xamarin Test Cloud)
The only way I managed to break it is when Xamarin.UITest is too fast (typing while the keyboard is opening, which makes the OS lag)

@Hardt-Coded
Copy link

Hi. I try to setup a project in the next days. I had a basic login form only with username and password. Nothing mor. Sometimes the behavior appeared in the emulator and sometime on my Huawei P20. The Behavior ist, that when I enter something it stops after a couple of letters and than the last letter apears and disapears in an I think invinite loop. You are not able to enter some thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t/bug Something isn't working as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants