Skip to content

Commit

Permalink
add IGListExperimentThrowOnInconsistencyException
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Maxime Ollivier authored and facebook-github-bot committed Oct 19, 2023
1 parent 5217e74 commit 5e79f8a
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 2 deletions.
4 changes: 3 additions & 1 deletion Source/IGListDiffKit/IGListExperiments.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
};

/**
Expand Down
5 changes: 4 additions & 1 deletion Source/IGListKit/Internal/IGListBatchUpdateTransaction.m
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 5e79f8a

Please sign in to comment.