Windowscommunitytoolkit: IncrementalLoadingCollection loads multiple pages on RefreshAsync

[ThomasPe]

I'm submitting a...

[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report 
[ ] Feature request
[ ] Sample app request
[ ] Documentation issue or request
[ ] Question of Support request => Please do not submit support request here, instead see https://github.com/Microsoft/UWPCommunityToolkit/blob/master/contributing.md#question

Current behavior

When I call RefreshAsync() on an IncrementalLoadingCollection the GetPagedItemsAsync() method gets called two or three times, depending on how long the web request takes. The pageIndex is being increased, but the requests sometimes finish out of order so the items are not correctly ordered as well.

Expected behavior

There should be only one concurrent GetPagedItemsAsync() at all times.

Minimal reproduction of the problem with instructions

I'm using this here: https://github.com/wp-net/WordPressUWP
The collection is in the WordPressUWP/WordPressUWP/ViewModels/NewsViewModel.cs

Environment

Nuget Package(s): 

Package Version(s): 

Windows 10 Build Number:
- [ ] Anniversary Update (14393) 
- [ ] Creators Update (15063)
- [x] Fall Creators Update (16299)
- [ ] Insider Build (xxxxx)

App min and target version:
- [x] Anniversary Update (14393) 
- [ ] Creators Update (15063)
- [ ] Fall Creators Update (16299)
- [ ] Insider Build (xxxxx)

Device form factor:
- [x] Desktop
- [ ] Mobile
- [ ] Xbox
- [ ] Surface Hub
- [ ] IoT

Visual Studio 
- [ ] 2017 15.3
- [x] 2017 15.4

[nmetulev]

Hey @ThomasPe, is there a good wordpress site I can use to repro the issue?

[ThomasPe]

any with multiple posts should do, I'm currently using https://windowsarea.de/wp-json/

[windowstoolkitbot]

This issue seems inactive. Do you need help to complete this issue?

[ThomasPe]

Still an issue for me.

[nmetulev]

Haven't had time to look into this, will have to look at it after the new year.

@marcominerva might be able to help here though, Marco, any thoughts on this?

[marcominerva]

Hi! I'm unable to reproduce the issue.

I have download the repo https://github.com/wp-net/WordPressUWP and used the URL https://windowsarea.de/wp-json/. I have modified the GetPagedItemsAsync method adding a Delay to simulate long requests:

public async Task<IEnumerable<Post>> GetPagedItemsAsync(int pageIndex, int pageSize, CancellationToken cancellationToken = default(CancellationToken))
{
    Debug.WriteLine("GetPagedItemsAsync");
    await Task.Delay(1000);
    var result = await _wordPressService.GetLatestPosts(pageIndex, pageSize);
    Debug.WriteLine("EndGetPagedItemsAsync");

    return result;
}

But all the requests seem to be completed in order:

GetPagedItemsAsync
loading page 1
EndGetPagedItemsAsync
GetPagedItemsAsync
loading page 2
EndGetPagedItemsAsync
GetPagedItemsAsync
loading page 3
EndGetPagedItemsAsync
GetPagedItemsAsync
loading page 4
EndGetPagedItemsAsync

If you try this approach, do you obtain a different result?

[windowstoolkitbot]

This issue seems inactive. Do you need help to complete this issue?

[nmetulev]

@ThomasPe, is this still an issue? It doesn't look like we can reproduce the issue.

[ThomasPe]

yes, I am still seeing this. It might need a few tries, but eventually you'll run into a case where the second request is just a bit faster than the first.

The real problem here is, that there shouldn't even be a second call, right?

[marcominerva]

It is correct that there is a second call, because the system might decide to invoke the method multiple times in a sort of "prefetching".

Have you tried adding the debug messages I shown you and have you verified that in the Output Window the results are out of order?

[ThomasPe]

I run into this issue when refreshing the collection. I do seem to get the responses in the correct order, but when a 3rd page is being loaded immediately (happens ~50% of the time) the items are out of order - not sure yet if maybe page 2 gets put at the beginning of the list.

GetPagedItemsAsync
loading page 0
GetPagedItemsAsync
loading page 1
GetPagedItemsAsync
loading page 2

[marcominerva]

As you can see at line https://github.com/Microsoft/UWPCommunityToolkit/blob/e97cb03c904c63ae39b3b1788cb0608447ca6e69/Microsoft.Toolkit.Uwp/IncrementalLoadingCollection/IncrementalLoadingCollection.cs#L260 of IncrementalLoadingCollection implementation, elements are added to the list with a foreach loop, so they are always appended at the end of the list.

[ThomasPe]

I have added a debug statement after the post returns for clarification. This is what happens from time to time since the requests do not finish at the same speed. As far as I can tell this only happens when I trigger await Posts.RefreshAsync();

loading page 0
loading page 1
return page 1
loading page 2
return page 0
return page 2

[marcominerva]

According to these commits:

• https://github.com/Microsoft/UWPCommunityToolkit/commit/d8a9df60cbe8f28e09429c061bd3dcc0da7c675d#diff-df5a5a86e220ff711acb585193e2be15
• https://github.com/Microsoft/UWPCommunityToolkit/commit/392e5c20718de34d06057f32628215411829b919#diff-df5a5a86e220ff711acb585193e2be15

@ianier added the Refresh method. Perhaps he can help you further.

[nguyenkien]

I facing same problems.
Below is my workaround:

IncrementalLoadingCollection
private async Task<LoadMoreItemsResult> LoadMoreItemsAsync(uint count, CancellationToken cancellationToken)
{
if (IsLoading) return new LoadMoreItemsResult();
uint resultCount = 0;
....
}

[windowstoolkitbot]

This issue seems inactive. Do you need help to complete this issue?

[ianier]

I'm too busy at the moment to do a PR. It looks like reentrancy is the issue. Protecting LoadMoreItemsAsync with a lock (e.g. AsyncLock from https://github.com/StephenCleary/AsyncEx) would be a good idea., but a simple flag may do the trick as well, as suggested by @nguyenkien.

[ThomasPe]

One problem with @nguyenkien 's solutions is that this will set the .IsLoading property of the IncrementalLoadingCollection to false prematurely - before the first request has been finished. So you would need to handle this yourself as well.

[ThomasPe]

@ianier using AsyncLock seems to be the best fix for now, thanks!

[nmetulev]

Thanks for looking into this folks. @ThomasPe, fancy submitting a PR to fix this?

[ThomasPe]

this is something I've wrapped around my own methods for now. I can certainly look at how this could be added to the Toolkit directly.

[windowstoolkitbot]

This issue seems inactive. Do you need help to complete this issue?

[nmetulev]

Hey @ThomasPe, have you had a chance to look into adding a fix to the toolkit? Code freeze for next release is next week, hoping we could get this in before then :)

[ThomasPe]

I just took another look and I think I found the real issue - which should be way easier to fix than doing some async lock.

https://github.com/Microsoft/UWPCommunityToolkit/blob/a3aa6d9f8cd09b12c31a1a3d465de1f24d327193/Microsoft.Toolkit.Uwp/IncrementalLoadingCollection/IncrementalLoadingCollection.cs#L213

Within the RefreshAsync() method the LoadMoreItemsAsync() is being called, triggering an extra (unnecessary) call to the LoadMore-Method. This is already being triggered by clearing the list.

I'll do a PR so you can confirm.

cc @ianier

[nmetulev]

PR merged, thanks @ThomasPe

[ianier]

FYI, the reason to call LoadMoreItems(1) was that just triggering a HasMoreItems property change wasn't enough for a ListView to initiate a refresh after the collection had been loaded in full. So if that ListView behavior (bug?) is still there, the new implementation won't cause the collection to reload in cases when HasMoreItems had been set to false at least once.

[nmetulev]

Argh, good to know, I'll test out the scenario and see if that behavior is still there

[nmetulev]

Just tested it with a collection being loaded all the way and HasMoreItems set to false. Refresh worked every time so it looks like the behavior/bug was fixed along the way :)