Blogging Prompts: Fix Core Data concurrency issues in scheduler by dvdchr · Pull Request #20854 · wordpress-mobile/WordPress-iOS

dvdchr · 2023-06-13T17:42:31Z

Fixes #20842

Warning:
Please note that this targets the release/22.6 branch, which is already in code freeze.

Description

When scheduling or rescheduling prompts, the related methods make a call to UNNotificationCenter to either request for push authorization or fetch notification settings. However, the notification center does not guarantee that the completion block will be executed in the caller's queue. Since the completion block code involves Core Data, this raises a multithreading violation when run in debug mode.

This PR fixes the issue by wrapping the Core Data-related code within these methods in the managed object context's queue.

Additionally, this also improves the thread-safety of BloggingPromptsService's init method by ensuring that any access to Core Data properties is performed within that object's context queue.

To test

From Xcode, launch the Jetpack app with -com.apple.CoreData.ConcurrencyDebug 1 launch argument.
Log in with your WordPress.com account.
Go to Site Menu > Site Settings.
Set Blogging Reminders with prompts enabled.
🔎 Verify that the reminders are successfully set and that no multithreading violation is raised.
Relaunch the Jetpack app from Xcode.
🔎 Verify that no multithreading violation is raised.
Go to Site Menu > Site Settings.
In the Blogging Reminders setting, uncheck all the days from the reminders.
🔎 Verify that the reminders are successfully set and that no multithreading violation is raised.

Additionally, please also smoke test Blogging Reminders — creating, updating, or removing schedules; with and without prompts.

Regression Notes

Potential unintended areas of impact
Should be none. The logic is kept as close to the original as possible to minimize impact.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manually tested the changes, and the existing unit tests are passing.
What automated tests I added (or what prevented me from doing so)
N/A

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

N/A — this PR doesn't contain UI-related changes.

Portrait and landscape orientations.
Light and dark modes.
Fonts: Larger, smaller and bold text.
High contrast.
VoiceOver.
Languages with large words or with letters/accents not frequently used in English.
Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
iPhone and iPad.
Multi-tasking: Split view and Slide over. (iPad)

…ion settings UNNotificationCenter's `getNotificationSettings` method does not guarantee that the completion block is called in the original queue. This ensures that the completion block code that touches Core Data is run within the context's queue, as obtained from the managed object.

peril-wordpress-mobile · 2023-06-13T17:47:38Z

	Warnings
⚠️	This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

wpmobilebot · 2023-06-13T18:11:42Z

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr20854-a9d5782
	Version	22.6
	Bundle ID	com.jetpack.alpha
	Commit	`a9d5782`
	App Center Build	jetpack-installable-builds #5025

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2023-06-13T18:11:45Z

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr20854-a9d5782
	Version	22.6
	Bundle ID	org.wordpress.alpha
	Commit	`a9d5782`
	App Center Build	WPiOS - One-Offs #5997

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

WordPress/Classes/Services/BloggingPromptsService.swift

crazytonyli

Pattern like blog.managedObjectContext.perform { doSomething(with: blog)} feels like a patch to me. We probably should use it sparsely. Because there is no guarantee the doSomething function only uses the passed-in blog instance in its calling thread.

For instance, a couple of call sites of processSchedule is changed to be called from the blog context thread. But the function itself accesses the blog in an API callback which I believe is called from the main thread.

I think, yes, this change would fix the particular crash shown in #20848, but it may cause other issues.

One solution on top of my head is refactoring the Blog reminder classes to not use Blog type. It can declare a value type struct BlogData (with a better name probably), which includes a few property the Blog Reminder component really cares about: objectID, site id, titile, etc. So that it's not restricted by Core Data's programming model, but also has the capability to change the Blog model with the objectID if needed.

WordPress/Classes/Services/BloggingPromptsService.swift

WordPress/Classes/Utility/Blogging Prompts/PromptRemindersScheduler.swift

Refactor `addLocalNotification` and `addStaticNotifications` to operate with value types instead of dealing with `Blog` directly. There are still loose property reads to `Blog` within the method, but `processSchedule` is private and its callers are guaranteeing that the method is called from the Blog's context queue. Further improvements will be made in the next iteration.

dvdchr · 2023-06-14T10:17:14Z

Thanks for the input, @crazytonyli!

[...] this change would fix the particular crash shown in #20848, but it may cause other issues.
[...] One solution on top of my head is refactoring the Blog reminder classes to not use Blog type.

Great idea, and I agree. I was considering this yesterday but realized that there are going to be a lot of changes. Since I'm targeting 22.6 (which is in code freeze), I'm thinking of keeping the impact of this PR minimal and doing a larger refactor separately in the next iteration.

However, it doesn't make sense if the improvement is flawed; as you've pointed out:

For instance, a couple of call sites of processSchedule is changed to be called from the blog context thread. But the function itself accesses the blog in an API callback which I believe is called from the main thread.

Thanks for catching that. To address this, I've made some improvements to the processSchedule (along with a few other methods) in 6f89aa7 so that the Blog object is no longer directly accessed from the API completion block. Although there are still some direct calls to the Blog object before the API call, at least the method is private and all of its callers ensure that the method is called from the Blog's context queue.

In the (near) future, the larger refactor will remove the blog parameter from most of the Blogging Reminders and Blogging Prompts methods, replacing them with siteID or a value type as per your and Jeremy's suggestion.

Anyways, can you give this a second pass, please? Thank you!

crazytonyli

Thanks for addressing my feedback. This PR looks good to me. 👍

It would be nice to see a proper refactor done to the Blog Reminder part of the app soon though 😺

WordPress/Classes/Services/BloggingPromptsService.swift

crazytonyli · 2023-06-14T23:11:23Z

WordPress/Classes/Utility/Blogging Prompts/PromptRemindersScheduler.swift

              maxDays > 0,
-              let maxDate = Calendar.current.date(byAdding: .day, value: maxDays, to: afterDate),
-              blog.dotComID != nil else {
+              let maxDate = Calendar.current.date(byAdding: .day, value: maxDays, to: afterDate) else {


Not related to this PR. But I didn't realise that Calendar.current—the calendar that user picked in the device's System Settings—is used a lot in the app. In most cases, we want the Gregorian calendar, not the calendar the user likes their phone to display.

Hmm, yeah, I think I didn't really think about it and just followed how it's done across the codebase. I'll consider fixing this during the refactor later. Thanks!

…prompts-scheduler-concurrency

mokagio · 2023-06-29T11:30:46Z

@crazytonyli I ended up here while looking into something else, just wanted to say that this sounds like a great idea:

One solution on top of my head is refactoring the Blog reminder classes to not use Blog type. It can declare a value type struct BlogData (with a better name probably), which includes a few property the Blog Reminder component really cares about: objectID, site id, titile, etc. So that it's not restricted by Core Data's programming model, but also has the capability to change the Blog model with the objectID if needed.

dvdchr added 3 commits June 13, 2023 23:54

Execute Core Data code in the right queue after push authorization

7dc0f5c

Improve thread safety of BloggingPromptsService init method

c5cf208

dvdchr added [Type] Enhancement Core Data Issues related to Core Data Blogging Prompts labels Jun 13, 2023

dvdchr added this to the 22.6 ❄️ milestone Jun 13, 2023

dvdchr self-assigned this Jun 13, 2023

dvdchr requested review from crazytonyli and hassaanelgarem June 13, 2023 17:42

salimbraksa reviewed Jun 13, 2023

View reviewed changes

WordPress/Classes/Services/BloggingPromptsService.swift Show resolved Hide resolved

crazytonyli reviewed Jun 14, 2023

View reviewed changes

WordPress/Classes/Services/BloggingPromptsService.swift Show resolved Hide resolved

WordPress/Classes/Utility/Blogging Prompts/PromptRemindersScheduler.swift Show resolved Hide resolved

crazytonyli mentioned this pull request Jun 14, 2023

Core Data: Fixes a Core Data Multithreading violation when unscheduling blogging reminders #20852

Closed

4 tasks

dvdchr added 3 commits June 14, 2023 16:38

Simplify the init method by utilizing the return value from performQuery

efdad6e

Call the completion block when the guard condition fails

241e0ed

dvdchr requested a review from crazytonyli June 14, 2023 10:17

dvdchr linked an issue Jun 14, 2023 that may be closed by this pull request

Core Data: Multithreading violation when retrieving reminder information #20842

Closed

crazytonyli approved these changes Jun 14, 2023

View reviewed changes

dvdchr added 2 commits June 15, 2023 16:10

Merge remote-tracking branch 'origin/release/22.6' into fix/blogging-…

d33aacb

…prompts-scheduler-concurrency

style: remove extra spacing

a9d5782

dvdchr merged commit 0c85ba6 into release/22.6 Jun 15, 2023

dvdchr deleted the fix/blogging-prompts-scheduler-concurrency branch June 15, 2023 10:21

dvdchr mentioned this pull request Jun 15, 2023

Core Data: Multithreading violation when retrieving reminder information #20842

Closed

mokagio mentioned this pull request Jun 16, 2023

Merge beta 22.6.0.1 #20892

Merged

4 tasks

hassaanelgarem mentioned this pull request Jun 21, 2023

Core Data: Fixes a Core Data Multithreading violation when unscheduling blogging reminders #20918

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blogging Prompts: Fix Core Data concurrency issues in scheduler#20854

Blogging Prompts: Fix Core Data concurrency issues in scheduler#20854
dvdchr merged 8 commits intorelease/22.6from
fix/blogging-prompts-scheduler-concurrency

dvdchr commented Jun 13, 2023 •

edited

Loading

Uh oh!

peril-wordpress-mobile bot commented Jun 13, 2023

Uh oh!

wpmobilebot commented Jun 13, 2023 •

edited

Loading

Uh oh!

wpmobilebot commented Jun 13, 2023 •

edited

Loading

Uh oh!

Uh oh!

crazytonyli left a comment

Uh oh!

Uh oh!

Uh oh!

dvdchr commented Jun 14, 2023 •

edited

Loading

Uh oh!

crazytonyli left a comment

Uh oh!

Uh oh!

crazytonyli Jun 14, 2023

Uh oh!

dvdchr Jun 15, 2023

Uh oh!

mokagio commented Jun 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

dvdchr commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

To test

Regression Notes

Uh oh!

peril-wordpress-mobile bot commented Jun 13, 2023

Uh oh!

wpmobilebot commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

crazytonyli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dvdchr commented Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crazytonyli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

crazytonyli Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

dvdchr Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

mokagio commented Jun 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dvdchr commented Jun 13, 2023 •

edited

Loading

wpmobilebot commented Jun 13, 2023 •

edited

Loading

wpmobilebot commented Jun 13, 2023 •

edited

Loading

dvdchr commented Jun 14, 2023 •

edited

Loading