-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[Bug][iOS] Concurrent issue leading to crash in SemaphoreSlim.Release in ObservableItemsSource #11853
Comments
@softlion Thanks for the feedback. Do you have a small example where reproduce the issue?. |
This are the possible changes: https://gist.github.com/jsuarezruiz/341a2eea56dd9205729c317912b0bee5#file-gistfile1-txt-L362 |
I'm unable to reproduce the issue in a demo project. |
I can also confirm that I have days where it does not happen at all. But suddenly when my system is under load I sometimes get these exceptions in the iOS simulator. I was using the ObservableRangeCollection by @jamesmontemagno and did add about 25 elements when the error occurs regularly. It happens during the ItemThresholdCommand of a CollectionView, so my guess was that it's on the main thread anyway. |
Ok I have a repro project ! I'll post it here. Btw there are 3 different issues triggered by the same bug.
2
3
|
Repro app: https://github.com/softlion/ReproFormsCollectionViewBug Tested on iOS 12.4 / iPhone 6 simulator. Does not happen on Android / UWP. This app will trigger one of the 3 above crashes, which comes from different synchronization bugs. |
Please fix it fast as i gave you a 1 line change solution + a repro app. I've PR only once in Forms and it's 4 years ago. I'm not sure how much time it will now require to: clone the repo / build a local nuget / add all forms projects to the repro app / make sure the issue still triggers / fix it / rebase the repo / create a fix branch / push / commit / pr / receive feedbacks / re-rebase / re-commit |
I also use CollectionView on a ContentPage. When I return to previous Page I always get the exception shown below. ◢ | $exception | {System.Threading.SemaphoreFullException: Adding the specified count to the semaphore would cause it to exceed its maximum count. at System.Threading.SemaphoreSlim.Release (System.Int32 releaseCount) [0x0004c] in /Library/Frameworks/Xamarin.iOS.framework/Versions/Current/src/Xamarin.iOS/mcs/class/referencesource/mscorlib/system/threading/SemaphoreSlim.cs:795 at System.Threading.SemaphoreSlim.Release () [0x00000] in /Library/Frameworks/Xamarin.iOS.framework/Versions/Current/src/Xamarin.iOS/mcs/class/referencesource/mscorlib/system/threading/SemaphoreSlim.cs:760 at Xamarin.Forms.Platform.iOS.ObservableItemsSource.Reload () [0x00098] in D:\a\1\s\Xamarin.Forms.Platform.iOS\CollectionView\ObservableItemsSource.cs:146 at Xamarin.Forms.Platform.iOS.ObservableItemsSource.CollectionChanged (System.Collections.Specialized.NotifyCollectionChangedEventArgs args) [0x00198] in D:\a\1\s\Xamarin.Forms.Platform.iOS\CollectionView\ObservableItemsSource.cs:129 at Xamarin.Forms.Platform.iOS.ObservableItemsSource.CollectionChanged (System.Object sender, System.Collections.Specialized.NotifyCollectionChangedEventArgs args) [0x000b7] in D:\a\1\s\Xamarin.Forms.Platform.iOS\CollectionView\ObservableItemsSource.cs:108 at System.Runtime.CompilerServices.AsyncMethodBuilderCore+<>c.b__7_0 (System.Object state) [0x00000] in /Library/Frameworks/Xamarin.iOS.framework/Versions/Current/src/Xamarin.iOS/mcs/class/referencesource/mscorlib/system/runtime/compilerservices/AsyncMethodBuilder.cs:1021 at Foundation.NSAsyncSynchronizationContextDispatcher.Apply () [0x00000] in /Library/Frameworks/Xamarin.iOS.framework/Versions/14.0.0.0/src/Xamarin.iOS/Foundation/NSAction.cs:178 at (wrapper managed-to-native) UIKit.UIApplication.UIApplicationMain(int,string[],intptr,intptr) at UIKit.UIApplication.Main (System.String[] args, System.IntPtr principal, System.IntPtr delegate) [0x00005] in /Library/Frameworks/Xamarin.iOS.framework/Versions/14.0.0.0/src/Xamarin.iOS/UIKit/UIApplication.cs:86 at UIKit.UIApplication.Main (System.String[] args, System.String principalClassName, System.String delegateClassName) [0x0000e] in /Library/Frameworks/Xamarin.iOS.framework/Versions/14.0.0.0/src/Xamarin.iOS/UIKit/UIApplication.cs:65 at Geometrics.AtomDownloader.iOS.Application.Main (System.String[] args) [0x00001] in C:\Project\XappsDev\AtomDownloaderXF\AtomDownloader.iOS\Main.cs:12 } | System.Threading.SemaphoreFullException |
We've just updated a handful of apps to 4.8, from 4.4 and 4.5, with no other NuGet packages and we are seeing the same issues. |
…ItemsSource - fixes xamarin#11853
@bruzkovsky have you updated your clone to apply the fix the xamarin forms 5 prerelease ? I tried to checkout the 5 branch and apply your fix but after that building fails:
|
Hi @softlion , sorry for the delay, busy week... |
…ItemsSource - refactored ObservableItemsSource and ObersvableGroupedSource to not use SemaphoreSlim - ObservableItemsSource and ObservableGroupedSource now use a copy of the original items source to keep it in sync with the state of the UICollectionView - fixes xamarin#11853
…ItemsSource - refactored ObservableItemsSource and ObservableGroupedSource to not use SemaphoreSlim - ObservableItemsSource now uses a copy of the original items source to keep it in sync with the state of the UICollectionView - fixes xamarin#11853
…ItemsSource - refactored ObservableItemsSource and ObservableGroupedSource to not use SemaphoreSlim - ObservableItemsSource now uses a copy of the original items source to keep it in sync with the state of the UICollectionView - fixes xamarin#11853
…ItemsSource - refactored ObservableItemsSource and ObservableGroupedSource to not use SemaphoreSlim - ObservableItemsSource now uses a copy of the original items source to keep it in sync with the state of the UICollectionView - added warnings for using BatchUpdate on the UICollectionView - fixes xamarin#11853
…ItemsSource - refactored ObservableItemsSource and ObservableGroupedSource to not use SemaphoreSlim - ObservableItemsSource now uses a copy of the original items source to keep it in sync with the state of the UICollectionView - added warnings for using BatchUpdate on the UICollectionView - fixes xamarin#11853
No I have not been able to fix the build issue. About your workaround, it duplicates the content of the list. This is not optimal. I do have an implementation of this for mvvmcross which is both efficient and safe. It does freeze the list state between updates but does not duplicate the items. |
@softlion so please share it then :) Strange that the items are duplicated. I tested the fix with a lot of rapid modifications to the list and didn't find any issue. But I still want to test it in our production app next week, maybe I'll find more errors there... |
…ItemsSource- refactored ObservableItemsSource and ObservableGroupedSource to not use SemaphoreSlim- ObservableItemsSource now uses a copy of the original items source to keep it in sync with the state of the UICollectionView- added warnings for using BatchUpdate on the UICollectionView- fixes xamarin#11853
We are seeing this issue as well using Xamarin.Forms 4.8. |
I have now successfully build a custom nuget package based on newest 4.8.0 branch combined with updates from @bruzkovsky. This fixes the issue we saw. Looking forward to an official update/release though :-) |
@mfmadsen could you share your github/branch with the updates ? Is it https://github.com/mfmadsen/Xamarin.Forms ? Ok i can't use it. I'm already using Xamarin.Forms 5 features. |
@softlion, yes that is the repo we are currently using to build our temporary custom nuget package for 4.8 Xamarin.Forms. I was never able to build the repo from @bruzkovsky directly, so instead I cloned the Xamarin.Forms repo and then replaced the contents of the three updated files in Xamarin.Forms.Platform.iOS/CollectionView (ListSource.cs, ObservableGroupedSource.cs and ObservableItemsSource.cs). After that I was able to build. It seems some changes was made in the original Xamarin.Forms repo that fixes the build issues we both were seeing when trying to build the branch from @bruzkovsky. |
EDIT: the custom code they put in XamarinForms 5 pre5 don't work at all. The one in the 5.0.0 branch don't work at all too. Among the issues: crash when the view is on a page in the stack (ie: another page has been pushed), crash randomly, displays no item (related to offscreen composition too). EDIT2: ok you really need me to fix this mess. Ok then i'll take 4 hours to fix this shit. EDIT3:
Also this version has a limitation to 1 item added at a time for animations to work. Otherwise it will simply reload the whole list, loosing the current scroll position. Making lazy loading impossible. Especially it does not support AddRange, RemoveRange, ReplaceRange, MoveRange. Nor collection change transactions. EDIT 4: easy repro: the collection stays empty if the control is in a page which is not attached to a parent yet, event if the ObservableCollection has data, and stays empty even after the page is attached (for example as a Detail of a MasterDetail page). @mfmadsen Good news the fix of your fix is in XamarinForms 5 pre5 (you used async for nothing in your fix).
And to compile it you need a fix to your VS as explained in this solution: https://developercommunity.visualstudio.com/content/problem/1256345/uwp-build-fails-after-update-to-1682-error-msb3774.html |
…alls that were messing up the internal UICollectionView bookkeeping; fixes #11853
I wonder if #13126, ie filling an ObservableCollectionView is related to this, as it shares several characteristics (toggling IsVisible alters behavior, the ObservableCollection sends only one update event which doesn’t draw properly and so on). Thank you @softlion for your work. It’s discouraging to see how many bugs you found in the “fixes”. Xamarin needs to step up their testing. It’s getting to a point where the expectation is that things won’t work :/ |
@softlion The regressions you mentioned in the 5.0.0 branch, is there any chance you could make a new bug report about them as I think they’ll get buried as this has now been closed? |
@Tommigun1980 @hartez completely rewrote a fix, removing all the new code from above, and the semaphore and all other thing. i have not tested it yet as I moved things in my customer app to workaround these issues. i hope it’s quality is better! ;) |
Oh good to hear, really looking forward to testing those fixes as there's a good chance they will fix #13126 also! Thanks for the info, I appreciate it. |
Description
Calls to
ObservableItemsSource.BatchUpdate
should be synchronized to prevent a crash in _batchUpdating.Release(), because this last one can be called 2 times in a row and will fail the second time.https://github.com/xamarin/Xamarin.Forms/blob/main/Xamarin.Forms.Platform.iOS/CollectionView/ObservableItemsSource.cs
Steps to Reproduce
I am able to reproduce this issue easily in my customer's app.
I've already checked that all calls are done on the main thread.
It happens nearly everytime when updating the bound ObservableCollection 3 times in a row:
1st deleting some items, 2nd inserting some new items, 3rd moving some existing items (it's an algorithm that compute the changes between an ObservableCollection and a "new" list, and apply the 3 above actions so after the ObservableCollection becomes equal to the list).
I've already checked that all 3 calls are done on the main thread.
Expected Behavior
don't crash
Actual Behavior
crash when releasing the SemaphoreSlim because its CurrentCount is already 1.
Stack trace:
Basic Information
Screenshots
Reproduction Link
Repro app: https://github.com/softlion/ReproFormsCollectionViewBug
Run the app on an iOS simulator or device.
Tested on iOS 12.4 / iPhone 6 simulator.
But happens on all iOS OS and devices.
Does not happen on Android / UWP.
This app will trigger one of the 3 above crashes, which comes from different synchronization bugs.
The key is the change of the IsVisible property just before or after items have been added to the collectionviewsource.
Workaround
None
Other infos
I've checked the xamarin forms source code.
I'm pretty sure you can fix it easily by making BatchUpdate return a Task, then start BatchUpdate() with
await _batchUpdating.WaitAsync();
instead of putting_batchUpdating.Wait()
inside thePerformBatchUpdates
action parameter.The text was updated successfully, but these errors were encountered: