From 5e79f8a3198349c0b5bc658e398f165fcaf79716 Mon Sep 17 00:00:00 2001 From: Maxime Ollivier Date: Thu, 19 Oct 2023 09:57:48 -0700 Subject: [PATCH] add IGListExperimentThrowOnInconsistencyException MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Currently, we don't throw on `NSInternalInconsistencyException` as a temporary workaround, but there's a few issues with that: 1) `IGFailure` doesn't collect nearly as much information as exceptions, so it's difficult to debug. We could add more context to it, but feels like we should just fix the underlying issue instead, and remove the workaround. 2) If we throw in an animated update, all app animations break because `[UIView performWithoutAnimation:...]` doesn’t close properly. 3) Continuing to use the `UICollectionView` after it throws feels a bit risky IMO. There's no guarantee that it should work properly after that. Lets reintroduce the exception slowly, fix each problem, and eventually remove the workaround. In the future, we could do 2 other things: 1) Make `IGListExceptionDoctor` that can look at an exception's details and diagnose the issue, or at least provide more context. 2) Check if the updates make sense before telling the `UICollectionView`, so we can fallback to a `-reloadData`. This would shift the problem from a crash to a performance regression, so not ideal, but maybe less worse for production. I'll add the experiment set-up in the next diffs. Differential Revision: D50426255 fbshipit-source-id: 26e21d3dfcf4670ed07f397cf0d4decdda08eec5 --- Source/IGListDiffKit/IGListExperiments.h | 4 +++- Source/IGListKit/Internal/IGListBatchUpdateTransaction.m | 5 ++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/Source/IGListDiffKit/IGListExperiments.h b/Source/IGListDiffKit/IGListExperiments.h index 72d6da40f..3ebb787dc 100644 --- a/Source/IGListDiffKit/IGListExperiments.h +++ b/Source/IGListDiffKit/IGListExperiments.h @@ -23,7 +23,9 @@ typedef NS_OPTIONS (NSInteger, IGListExperiment) { /// Test skipping creating {view : section controller} map, which has inconsistency issue. IGListExperimentSkipViewSectionControllerMap = 1 << 4, /// Use the correct section index when calling -[IGListAdapter reloadObjects ...] within the update block. - IGListExperimentFixCrashOnReloadObjects = 1 << 5 + IGListExperimentFixCrashOnReloadObjects = 1 << 5, + /// Throw NSInternalInconsistencyException during an update + IGListExperimentThrowOnInconsistencyException = 1 << 6 }; /** diff --git a/Source/IGListKit/Internal/IGListBatchUpdateTransaction.m b/Source/IGListKit/Internal/IGListBatchUpdateTransaction.m index 96ab93bcc..9cdf98b9c 100644 --- a/Source/IGListKit/Internal/IGListBatchUpdateTransaction.m +++ b/Source/IGListKit/Internal/IGListBatchUpdateTransaction.m @@ -217,7 +217,10 @@ - (void)_applyDiff:(IGListIndexSetResult *)diffResult { } } @catch (NSException *exception) { - if ([[exception name] isEqualToString:NSInternalInconsistencyException]) { + /// Currently, we don't throw on `NSInternalInconsistencyException`, like the comment below explains. This was a temporary workaround for the large + /// volume of exceptions that started with Xcode 14.3. Now, lets use this experiment flag to slowly reintroduce it, and eventually remove the workaround. + const BOOL ignoreException = !IGListExperimentEnabled(self.config.experiments, IGListExperimentThrowOnInconsistencyException); + if (ignoreException && [[exception name] isEqualToString:NSInternalInconsistencyException]) { /// As part of S342566 we have to recover from crashing the app since Xcode 14.3 has shipped /// with a different build SDK that changes the runtime behavior of -performBatchUpdates: issues. /// When we are performing batch updates, it's on us to advance the data source to the new state