fix 11853 - concurrency problem in ObservableItemsSource

[bruzkovsky]

Description of Change

This PR is a refactoring of the classes ObservableItemsSource and ObservableGroupedSource for two reasons,

1. the workarounds that grew in these classes eventually lead to Issue #11853 (and maybe others?), and
2. the code did not have optimal quality in both readability and control flow.

The workarounds were removed (including the incorrect usage of the SemaphoreSlim class) and the implementation was aligned more with the functionality of the underlying platform (UIKit.UICollectionView). The SemaphoreSlim was introduced in commit 7cfe6155, to fix a concurrency issue that could be easier fixed with this refactoring. Instead, it introduced #11853 which crashes the application. The concurrency problems that this commit fixed were mainly caused by commit 0a11a184, which unnecessarily wrapped all operations on the UICollectionView inside PerformBatchUpdates.

Additionally, after fixing the above, I noticed that there is a conceptual problem in the ObservableItemsSource, namely that there is an inconsistency between the ItemsSource and the actual items that the UICollectionView uses. Following example demonstrates the issue:

1. ItemsSource is changed.
2. UICollectionView processes the update asynchronously on the MainThread (possibly concurrent)
3. ItemsSource is cleared
4. UI CollectionView, while processing the previous update, requests the item that was changed -> but the collection is empty -> IndexOutOfRangeException

To address this issue, I created a field that holds a copy of the original ItemsSource, which is updated with every INotifyCollectionChange update to resemble the state of the collection before the update occurs. The original items source is only subscribed to and used when the Reset event is handled.

Issues Resolved

• fixes #11853

API Changes

None

Platforms Affected

• iOS

Behavioral/Visual Changes

Does not crash anymore when applying multiple changes to an ObservableCollection that is the ItemsSource of a CollectionView.

Before/After Screenshots

Not applicable

Testing Procedure

The test procedure is automated in the class Issue11853.cs. Just press the Start button and wait for the Success to appear. The test fails if the app crashes.

PR Checklist

• [ ] Targets the correct branch
• [ ] Tests are passing (or failures are unrelated)

[bruzkovsky]

I added a UI Test which executes the test steps, but could not run it because it said iOS tests are not supported on Windows:

OneTimeSetUp: App did not start for some reason: System.Exception: iOS tests are not supported on Windows.

I am connected to a Mac, but I guess I would have to check it out an run it locally on a Mac?

[bruzkovsky]

@gentledepp FYI

[rmarinho]

@bruzkovsky yeah you need to run the UITests on a Mac, on Windows you can only run tests for Android.

[bruzkovsky]

@rmarinho thanks for clarifying, I'll try that on Monday, then.

[bruzkovsky]

Hmm strange, when I try this manually in the ControlGallery it doesn't crash. @rmarinho are there any more details on the failed test? I still have to install a lot of stuff on my mac to get the XF solution to build, so I cannot run it myself yet.

[rmarinho]

@bruzkovsky

System.Exception : Error while performing WaitForElement(Marked("Success"), "Timed out waiting for element...", null, null, null)
----> System.TimeoutException : Timed out waiting for element... 

You shouldn't need lot's of things. also you can use the provisioning cake target to help you with missing bits.

[bruzkovsky]

@rmarinho I pushed new changes, could you please trigger the UITest again?

Unfortunately I can't run them locally... I get the following error: Failed to launch DeviceAgent

Output: https://gist.github.com/bruzkovsky/c8beb056e07ab68f4e024d803bfdf2a4

The DeviceAgent was installed on my device, but I cannot get this to run. Maybe someone can give me instructions?

[bruzkovsky]

Hi! So I didn't manage to run the UITests in VS for Mac at all, but now I could do something with the cake script. After running some minutes, it tells me this:

Test Run Summary
  Overall result: Passed
  Test Count: 768, Passed: 0, Failed: 0, Warnings: 0, Inconclusive: 768, Skipped: 0
  Start time: 2020-11-19 13:03:35Z
    End time: 2020-11-19 13:06:58Z
    Duration: 202.763 seconds

and the logs say:

19-11-2020 15:34:09.976 +01:00 - 0 - iOS test running Xamarin.UITest version: 3.0.7
19-11-2020 15:34:09.976 +01:00 - 0 - Skipping IDE integration as important properties are configured. To force IDE integration, add .PreferIdeSettings() to ConfigureApp.
19-11-2020 15:34:10.670 +01:00 - 694 - Artifact folder: /var/folders/gb/b9z4j62100vf04pjg_mfpt680000gn/T/uitest/a-D43DA6095BA2B9EA169F225244E843B634DDEAAF
19-11-2020 15:34:10.670 +01:00 - 694 - Using cached artifact folder: idevice-tools
19-11-2020 15:34:14.685 +01:00 - 4708 - 1 - Exception
Exception: System.Threading.ThreadAbortException: Thread was being aborted.
  at (wrapper managed-to-native) System.Threading.Monitor.Monitor_wait(object,int)
  at System.Threading.Monitor.ObjWait (System.Boolean exitContext, System.Int32 millisecondsTimeout, System.Object obj) [0x0002f] in <3f2977eb306b45388f224fc6cc4db2c4>:0 
  at System.Threading.Monitor.Wait (System.Object obj, System.Int32 millisecondsTimeout, System.Boolean exitContext) [0x0000e] in <3f2977eb306b45388f224fc6cc4db2c4>:0 
  at System.Threading.Monitor.Wait (System.Object obj, System.Int32 millisecondsTimeout) [0x00000] in <3f2977eb306b45388f224fc6cc4db2c4>:0 
  at System.Threading.ManualResetEventSlim.Wait (System.Int32 millisecondsTimeout, System.Threading.CancellationToken cancellationToken) [0x00100] in <3f2977eb306b45388f224fc6cc4db2c4>:0 
  at System.Threading.Tasks.Task.WaitAllBlockingCore (System.Collections.Generic.LowLevelListWithIList`1[T] tasks, System.Int32 millisecondsTimeout, System.Threading.CancellationToken cancellationToken) [0x00038] in <3f2977eb306b45388f224fc6cc4db2c4>:0 
  at System.Threading.Tasks.Task.WaitAll (System.Threading.Tasks.Task[] tasks, System.Int32 millisecondsTimeout, System.Threading.CancellationToken cancellationToken) [0x000e6] in <3f2977eb306b45388f224fc6cc4db2c4>:0 
  at System.Threading.Tasks.Task.WaitAll (System.Threading.Tasks.Task[] tasks, System.Int32 millisecondsTimeout) [0x00000] in <3f2977eb306b45388f224fc6cc4db2c4>:0 
  at System.Threading.Tasks.Task.WaitAll (System.Threading.Tasks.Task[] tasks) [0x00000] in <3f2977eb306b45388f224fc6cc4db2c4>:0 
  at Xamarin.UITest.XDB.Services.Processes.ProcessService.Run (System.String command, System.String arguments) [0x0004f] in <d6deb34c8911492ebfea02a0239d2ab8>:0 
  at Xamarin.UITest.XDB.Services.OSX.XcodeService.TestWithoutBuilding (System.String deviceId, System.String xctestrunPath, System.String derivedDataPath) [0x0002b] in <d6deb34c8911492ebfea02a0239d2ab8>:0 
  at Xamarin.UITest.XDB.Services.OSX.iOSDeviceManagerService.StartTest (System.String deviceId) [0x00022] in <d6deb34c8911492ebfea02a0239d2ab8>:0 
  at Xamarin.UITest.XDB.Services.iOSDeviceAgentService+<>c__DisplayClass18_0.<LaunchTestAsync>b__2 () [0x00000] in <d6deb34c8911492ebfea02a0239d2ab8>:0 
  at Xamarin.UITest.XDB.Services.iOSDeviceAgentService.WithErrorHandling (System.Int32 eventId, System.Func`1[TResult] action, System.String errorMessage, System.Int32[] successCodes) [0x0000e] in <d6deb34c8911492ebfea02a0239d2ab8>:0 

[PureWeen]

@bruzkovsky unfortunately UI Test isn't currently compatible with Xcode 12 :-/ There was a new release of Xamarin.UITest yesterday but it's still not working for me

In order to make it work you have to install Xcode 11 and this https://download.visualstudio.microsoft.com/download/pr/68ffa29a-6a5b-41f7-af7b-506ddcf4bbfc/35159cac3be1910e87309c0094a8ec8a/xamarin.ios-13.18.2.1.pkg

It's a bit of a headache :-/ and hopefully the XCode 12 issue will be resolved soon

Until then your best bet is to just walk through the test manually to try and reproduce

[bruzkovsky]

@PureWeen thanks for the confirmation! Now I at least know what's going on. Could you just tell me which tests are failing? I fixed the AddingItemToUnviewedCollectionViewShouldNotCrashby manually running the repro, but the check says there are 6 failing tests.

On the other hand, while testing my custom nuget package, I found another issue that was introduced with my changes to ObservableGroupedSource. Basically the binding is completely broken. I'll have to investigate this, until then please do not merge this PR!

[gentledepp]

@bruzkovsky Could you please comment your PR so we understand the changes? I tried to understand why you did those changes and failed to do so :-|

[bruzkovsky]

On the other hand, while testing my custom nuget package, I found another issue that was introduced with my changes to ObservableGroupedSource. Basically the binding is completely broken. I'll have to investigate this, until then please do not merge this PR!

Hi @rmarinho, @PureWeen The issue I pointed out above was that I forgot the case when an ItemsSource of ObservableCollection has groups of ObservableCollection. I fixed this with the last changes and it works now. I'm still waiting for someone to tell me which tests are failing, because I don't have access to the results.

@gentledepp good suggestion, I'll make sure to comment the changes.

Edit: @rmarinho do you want me to retarget to 5.0.0?

[bruzkovsky]

@PureWeen as you suggested I ran through all test cases containing "CollectionView" and did not find any issues. I also tested my changes in our production app and no issues there either. There are also some guys using my changes in a custom package (see #11853). Please someone find some minutes to review this 🙏 @rmarinho @hartez @jsuarezruiz @rmarinho

If you want me to target 5.0.0 before reviewing please tell me. I don't care because I'll use my custom 4.8.0 package anyway, for now.

[rmarinho]

@bruzkovsky we made some changes related with this on 5.0.0, can you try 5.0.0 see if this fixes your issue?

[bruzkovsky]

@rmarinho It's still there in 5.0.0.1709-pre4

[jsuarezruiz]

@bruzkovsky Could you rebase your branch to fix the conflicts?. Thanks!

[roflkins]

@bruzkovsky I cannot tell you how thankful I am to have found this PR and love your work.

Been pulling my hair out for days trying to work around the current implementation of ObservableGroupedSource when its source is an ObservableCollection that is bound to DynamicData. It uses the DynamicData's Switch() extension to switch streams, which on emission streams in a series of remove on sections and items to clear the old items out of ObservableCollection. When it did that, and CollectionView.IsGrouped is true, I'd get varying "Objective-C exception thrown. Name: NSInternalInconsistencyException Reason: attempt to delete item 2 from section 0 which only contains 2 items before the update" exceptions. Even though the observable was ObserveOn(RxApp.MainThreadScheduler).

When I apply your changes, those exceptions completely disappeared and I can finally continue. I hope your PR eventually gets merged in.

[bruzkovsky]

@roflkins thank you for your kind words, I appreciate it. I currently don't work with XF anymore, focusing on Web, so it's up to someone else to fix the conflicts and merge this in. But maybe it would be wiser to look into the MAUI CollectionView. KR

[jfversluis]

Thanks for this suggestion! As Xamarin.Forms is now in maintenance mode, this will not happen anymore for Xamarin.Forms. We're only adding bugfixes and stability fixes.

If this is still important to you, make sure to check the .NET MAUI repo and see if it's already fixed there.