Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CBL-5074 : Fix backgrounding logic to cover conflict resolution #3226

Merged
merged 2 commits into from
Feb 13, 2024

Conversation

pasin
Copy link
Contributor

@pasin pasin commented Feb 13, 2024

  • Cherry-Picked the fixes from lithium branch (9f194b8 and 0f33a19) for CBSE-15077. There were conflicts during the cherry-pick but they are easy to resolve. Below are the commit messages copied from the original fixes.

  • The extending background task shouldn’t be ended if there are still pending conflict resolutions.

  • When the app was suspended by the background monitor, any current pending conflict resolutions should be canceled. All cancelled conflict resolutions will be notified and put into the queue again when the replicator is resumed or is restarted.

  • Fixed bug that the backgrounding monitor may not be restarted after it was ended.

  • Renamed some methods and add comments to code.

  • Added test to test suspending conflict resolution.

  • (Additional Commit) Fix EE Test build error when using XCode 15 and a flaky condition in testCollectionConflictResolver test.

@pasin pasin force-pushed the fix/CBL-5074 branch 2 times, most recently from a6c7ffc to ccb553a Compare February 13, 2024 02:06
* The extending background task shouldn’t be ended if there are still pending conflict resolutions.

* When the app was suspended by the background monitor, any current pending conflict resolutions should be canceled. All cancelled conflict resolutions will be notified and put into the queue again when the replicator is resumed or is restarted.

* Fixed bug that the backgrounding monitor may not be restarted after it was ended.

* Renamed some methods and add comments to code.

* Added test to test suspending conflict resolution.
@pasin pasin requested review from velicuvlad and bmeike February 13, 2024 04:39
Copy link
Contributor

@bmeike bmeike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't really read all of it. That parts I can understand look right.

@@ -351,6 +351,12 @@
27F9619A1ED8D9440060F804 /* CBLReachability.h in Headers */ = {isa = PBXBuildFile; fileRef = 27F961971ED8D9440060F804 /* CBLReachability.h */; };
27F9619B1ED8D9440060F804 /* CBLReachability.m in Sources */ = {isa = PBXBuildFile; fileRef = 27F961981ED8D9440060F804 /* CBLReachability.m */; };
27F9619C1ED8D9440060F804 /* CBLReachability.m in Sources */ = {isa = PBXBuildFile; fileRef = 27F961981ED8D9440060F804 /* CBLReachability.m */; };
40BC51EF2AFC39ED0090EDD5 /* CouchbaseLite.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 9343F00F207D611600F19A89 /* CouchbaseLite.framework */; };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these things? I've seen them enough, now, so that I'm pretty curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's CouchbaseLite framework used in the test project. XCode 15 just moves the line that defines that in the project file. It's fine there.

@@ -379,6 +386,15 @@ - (void) safeBlock:(void (^)())block {
}
}

// Always being called inside the lock
- (void) idled {
Assert(_rawStatus.level == kC4Idle);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also curious about this...
What does "Assert" do? Does it throw? Does it throw only if debugging?

Copy link
Contributor Author

@pasin pasin Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a config to disable the exception thrown in release mode. I have never tested the config so I think it's a good idea to test it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out Assert() always thrown. We have used them everywhere in Objective-C code. I will leave the code as is and will have separate ticket to review them all to see which ones we want to throw only in debug mode.

@pasin pasin merged commit 0686541 into release/3.1 Feb 13, 2024
6 checks passed
@pasin pasin deleted the fix/CBL-5074 branch February 13, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants