runtime: Preview 6 regression with AddRange on ICollection/Collection
When updating the SDK from Preview 5 to the latest Preview 6 nightly (3.0.100-preview6-012131) and recompiling my app, I started getting runtime exceptions in ListCollectionView due to changes in AddRange.
I have an extension method keyed on ICollection<T> called AddRange that accepts an IEnumerable<T>. It loops over the items and calls Add(T item) on the collection. I believe this is a common extension method people have added for convenience.
The trouble is that the new implementation of AddRange on Collection<T> does the add as a bulk insert at the end. This changes the behavior for consumers like the WPF ListCollectionView. That type listens for property changes on ObservableCollection<T>, but it does not support bulk operations, and throws.
https://github.com/dotnet/wpf/blob/master/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Data/ListCollectionView.cs#L2521
The net effect here is that a common helper method that is used in many WPF apps (AddRange to an ObservableCollection<T> no longer binds and the replacement method’s behavior has a negative and observable side effect.
The workaround is to create an AddRangeScalar method with the same signature as the original AddRange extension method and ensure that’s always called. However, that has a wide impact on existing code and it’s really easy for people to miss it and trigger a runtime exception on that code path.
About this issue
- Original URL
- State: closed
- Created 5 years ago
- Reactions: 1
- Comments: 16 (14 by maintainers)
Thanks @ahoefling – this is not your fault or anyone else’s this was just bad timing and unfortunately we don’t have time now to do the proper fix before Preview 6. However, be sure that I will follow up with the WPF guys to take action so that we can try and get this into Preview 7 and if we can sort it out, the way that we will do it is by Reverting my Revert 😄.
Also, we don’t need to revert anything in coreclr as of now, since that is just implementation. As long as we don’t expose this APIs in the reference assembly we’re fine, so that is why just reverting corefx is enough.
TBH, when I first proposed the API I was afraid there were going to be farther-reaching changes than people anticipated. A lot of code was based on flawed implementations on how to handle those events (the original implementation of observable incorrectly accounted for the possibility of multiple events to be fired at once, for example).
It’s going to have a lot farther downstream changes than just this. My concern would be that if you roll back now and then put it back in for Preview 7, then you’ll find other downstream breakages too late in the game for the 3.0 RTM. I realize nobody asked my opinion, but I would think it would be better to document the change now, break people, and then work with the teams to fix the broken downstream stuff for the next preview.
I realized I said it only in email – but HUGE kudos to @onovotny for saving us from shipping these bits with such bad WPF experience. Thank you! You’re savior of the day (as usual) 😉 Thanks!!!
Next time we meet, I am buying 😉
@ahoefling if you (or anyone) is up to it – debugging through WPF, figuring out what went wrong there and suggesting solutions in WPF code would be very appreciated. It would save mainly WPF team’s time … which is precious as they have plenty of things to finish for .NET Core 3.0, more than CoreFX team has.
And thanks a lot for your offer! We really appreciate it! As @safern mentioned, this is NOT your fault at all … we all missed it and the key is to react now 😃
@karelz and team, I am sorry this was missed during the Pull Request and the months of hard work we all did to get this into tn to CoreFX and NET Standard.
What can I do to help in the meantime so we can get this right for Preview 7. I am worried that we aren’t going to be able to get this working and it is going to be excluded when .NET Core 3.0 is Generally Available. I understand this is a convenience API but I am willing to help out since I contributed this change.
Note on the change
The change was committed in many PRs in both CoreFX and CoreCLR. I am not sure if it is required to revert all PRs or just the 1 cited above. Here is my list of PRs attached to the original issue
What can I do to as far as testing scenarios to try and get the issues resolved for Preview 7?
NuGet Package Explorer shows it in many places. Clone master and run with preview6 (before this fix). First place I noticed is in the “open from feed” dialog where there was an error message instead of displaying the feed info. There are other places too.
@onovotny Can you post a WPF app that exhibits the behavior somewhere so I can debug through it? Maybe I can track down the issue and fix it in the ListCollectionView.
Thanks!
Thanks @safern! That may be best option - can you please send red-bang email to WPF folks? (Vatsan, Amit, Fabian above)
cc @wtgodbe - another last minute must-have change in Preview 6.
I wonder if we should just try to fix up WPF
ListCollectionViewinstead of backing out the change …@safern can you please take a look? This may impact our ability to get feedback on WPF apps. At minimum it will be blocker for Preview 7.
cc @danmosemsft @vatsan-madhavan @amit-kabra @fabiant3 @ericstj