Adopt the new Core Data writer API#19185
Conversation
crazytonyli
changed the title
You can test the changes in Jetpack from this Pull Request by:
|
You can test the changes in WordPress from this Pull Request by:
|
jkmassel
left a comment
jkmassel
left a comment
There was a problem hiding this comment.
I have some concerns about the Task-based approach, which I've noted inline. I hope it makes sense, and I'm happy to discuss it further!
One other thing that comes to mind – I'm wondering about save as the "write" method name. Sorry if this is an over-nitpick, but for a situation with more complex write needs IMHO the original performAndWait is actually a bit more clear here – the developer is being given a context for whatever their needs might be. That's also the case with save, but I could see an argument for "you're doing Blog.lookup in the save method – that's not right!" based on the naming. I had originally suggested performBackgroundOperationAndSave to hopefully better explain the usage inline, though it's certainly wordy 😅. I wonder if performAndSave (extended from NSManagedObject.performAndWait) would be clearer?
Leaving as "Request Changes" for discussion!
| } | ||
| func saveSettings(_ remoteSettings: RemoteBloggingPromptsSettings, completion: @MainActor @escaping () -> Void) { | ||
| Task { | ||
| await contextManager.save { derivedContext in |
There was a problem hiding this comment.
I'm not sure we can use contextManager inside a Task, as we have no guarantee it won't be mutated outside the Task object 😔.
There was a problem hiding this comment.
Good point! I completely forgot about Sendable compliance, which I assume is your concern here?
There was a problem hiding this comment.
So...yes and no 😅. Sendable conformance should confirm what we're after (though it IMHO must be true conformance, not @unchecked), which I actually forgot was a thing! But can an Obj-C class have true Sendable conformance if the Swift compiler can't see its internals?
There was a problem hiding this comment.
Very true. Probably yet another reason to convert Objective-C to Swift 😄
| settings.configure(with: remoteSettings, siteID: self.siteID.int32Value, context: derivedContext) | ||
|
|
||
| self.contextManager.save(derivedContext) { | ||
| DispatchQueue.main.async { |
There was a problem hiding this comment.
This is an unfortunate API design that I'm not sure we should bake into the new method signature – it's never a good idea to make assumptions about where a callback will happen (if we chain this with other methods, it'll non-deterministically consume main thread time, which can make it nearly impossible to maintain scrolling/animation performance).
While a slightly better method signature could be something like:
func saveSettings(_ remoteSettings: RemoteBloggingPromptsSettings, completion: @escaping () -> Void, queue: DispatchQueue = .main) {it doesn't actually solve our problem, which is that when using Task, the only safe way to execute a callback is on on the MainActor.
IMHO we should really retain the callback-based approach here, and if/when we're ready to adopt async/await, we add new methods alongside callback-based ones and use those from the UI layer on down. I'm just not sure that it's possible to safely mix callbacks and async/await in safe and performant way?
There was a problem hiding this comment.
it's never a good idea to make assumptions about where a callback will happen
+1. Unless the callback has to run on a particular thread. That's usually the case for UI code that needs to run on the main thread, but should not be the case here, in "service" code.
our problem, which is that when using
Task, the only safe way to execute a callback is on on theMainActor.
@jkmassel the way I'm reading "safe" is "be guaranteed it runs on the main thread". My understanding is that we can run callback from a Task just by calling (in case of a closure callback()) them. But that's "not safe" because there's no guarantee on which actor / thread they'll run.
I think modern Swift allows caller to use the @MainActor annotation to tell the build system to run async code on the main thread. Unfortunately, we find ourselves in a mixed environment with different layers of legacy code from different times. Because of that, I think the suggestion of injecting the DispatchQueue in this method's signature is a good, small step to help us bridge the gap.
I'm just not sure that it's possible to safely mix callbacks and async/await in safe and performant way?
Does "safely" here refer to thread safety, or "so that they run on the thread that we want / would expect"? Also, what is the performance concern? Hopefully we'll have better performance monitoring via Sentry soon. 🙏
There was a problem hiding this comment.
. Because of that, I think the suggestion of injecting the
DispatchQueuein this method's signature is a good, small step to help us bridge the gap.
On the other hand... If a consumer has a specific requirement on the thread a callback should run, should it be their responsibility to specify that thread, because of the "make no assumptions rationale"?
Is passing a queue as part of the method signature better or worse than implicitly expecting callers to do something like
service.saveSettings(settings) {
DispatchQueue.main.async {
// completion code I want to run on the main thread?
}
}The upside of the "make no assumption" approach is that it's clean. The downside is that it might be easy to forget about the main thread.
Maybe an alternative is to require the DispatchQueue parameter, but not provide a default value, so that every caller is forced to think about threading.
Oh... Async is hard 🙃
There was a problem hiding this comment.
IMO, whether a closure argument is going to be called from the main thread or a background thread should be part of the API, either by documenting it or making it part of the function signature. A couple of examples like this are URLSession and NSManagedContextObject. So I agree with the "make no assumption" rule, since the API should make it explicit.
There was a problem hiding this comment.
@mokagio – just to address your question:
Does "safely" here refer to thread safety, or "so that they run on the thread that we want / would expect"? Also, what is the performance concern? Hopefully we'll have better performance monitoring via Sentry soon. 🙏
Thread safety, in this case. The only "certain" thread is the main one (and as you noted, the thread that services @MainActor). But blocking the main thread to sync background threads will have terrible performance implications, which was why I noted it.
| } | ||
| Task { | ||
| await ContextManager.shared.save { derivedContext in | ||
| let likers = remoteLikeUsers.map { remoteUser in |
There was a problem hiding this comment.
While it's not exceedingly likely that these objects will be mutated concurrently, it is possible here. It's (seemingly) stable under a queue-based concurrency model, but I'm not sure it's worth switching models until/unless we can guarantee thread safety.
Interested to hear y'alls thoughts here 🙂
There was a problem hiding this comment.
My understanding is ContextManager.shared.save is called by a random queue managed by Swift's structured concurrency, whereas the closure here is called inside an embedded "NSManagedObjectContext queue" from derivedContext.perform? So, I don't think the way that this remoteLikeUsers.map statement is called is changed?
There was a problem hiding this comment.
Right, sorry, I probably could've made this a bit clearer – so this methods' first argument remoteLikeUsers (btw, accepting an optional array is also bad API design IMHO) is an array of reference types. Thus, they could be called in a manner similar to:
commentService.createNewUsers(from: likeUsers, commentID: 1, siteID: 1, purgeExisting: false) {
// Do some stuff
}
likeUsers.each { user in
user.isCreating = true
}It doesn't work this way currently, but because these are reference types we IMHO can't use them inside a Task this way because we have no guarantees about their state?
| derivedContext.perform { | ||
| purgeStaleLikes(fromContext: derivedContext) | ||
| ContextManager.shared.save(derivedContext) | ||
| ContextManager.shared.save { |
There was a problem hiding this comment.
This is IMHO the ideal replacement strategy – tiny changes that don't adjust the overall approach or semantics of the operation, just consolidate the complexity into the ContextManager. It's definitely not as forward-thinking, but it's possible to know just by looking at it that it's safe.
| let context = manager.newDerivedContext() | ||
|
|
||
| context.performAndWait { | ||
| ContextManager.shared.save { context in |
There was a problem hiding this comment.
This (and the example below) are likewise nice simple changes
| let likers = remoteLikeUsers.map { remoteUser in | ||
| LikeUserHelper.createOrUpdateFrom(remoteUser: remoteUser, context: derivedContext) | ||
| } | ||
| Task { |
There was a problem hiding this comment.
I have the same concerns with a Task-based approach here
@jkmassel I agree, but for the new writer API, my understanding is it should only be used for writing (which is why I used The other reason I used "save" is so that the implementation can call the right |
|
@jkmassel I've changed this PR to use the completion block instead, to keep the diff at minimal. I'm keen to keep chatting about the potentially issues with using async API though, so please feel free to leave your comments. |
Correct! That part is great
Same – I've heard it's a hard thing 😁. If you're good with The changes so far address my concerns, but I'd still love an extra set of eyes from at least @mokagio on all this if possible! |
| @@ -94,22 +94,17 @@ private extension CommentService { | |||
| return | |||
There was a problem hiding this comment.
GitHub won't let me comment above this line, but line 93 is potentially an error – there's no requirement that createNewUsers is called from the main thread, thus onComplete can sometimes run on the main thread and sometimes run on the calling thread.
I'm not sure what to suggest, if anything – I agree with you preserving the original behaviour further down (manually specifying a queue), but I'm torn on whether we should fix this inconsistency. One approach might be precondition (NSThread.isMainThread, "This method must be called from the main queue")?
Curious about your thoughts (and whether you feel we should even be bothering to deal with it right now)
There was a problem hiding this comment.
I intentionally left this part untouched. Having looked at the code again, I feel like it should be okay to fix this inconsistency. I don't see this change would have any effect on the caller. But if it does, then it's likely something else requires a fix.
| - (void)saveUsingBlock:(void (^)(NSManagedObjectContext *context))aBlock; | ||
| - (void)saveUsingBlock:(void (^)(NSManagedObjectContext *context))aBlock completion:(void (^)(void))completion; | ||
| - (void)performAndSaveUsingBlock:(void (^)(NSManagedObjectContext *context))aBlock; | ||
| - (void)performAndSaveUsingBlock:(void (^)(NSManagedObjectContext *context))aBlock completion:(void (^)(void))completion; |
There was a problem hiding this comment.
Same – I've heard it's a hard thing 😁. If you're good with
performAndSave, I am too – great artists steal, and all that!
LGTM.
Well... I thought of performThenSave... just to make it clear which happens first, but maybe I'm splitting hairs here 😅 .
mokagio
left a comment
mokagio
left a comment
There was a problem hiding this comment.
Looking good, thanks Tony!
I think requiring a DispatchQueue as a method parameters in all those methods that are currently dispatching via DispathcQueue.main.async would be a good step to surface this concern and make is easier to upgrade the code to more recent Swift async/concurrent paradigms in the future. My rationale is that by removing implicit dependencies the code is more parametric and should be easier to convert, but I've never done it so I might be wrong.
I don't think those changes necessarily need to happen in this PR, also because they might result in a big footprint if the methods we'd update have many callers.
mokagio
dismissed
jkmassel’s stale review
While @jkmassel didn't give an explicit approval after his feedback was acted upon, he wrote "these changes address my concerns", #19185 (comment)
4 participants
This PR is part of #15830.
Changes
This PR replaces a few usages of
newDerivedContextwith the new writer API introduced in #19169.In this PR, I've taken advantage of the nicely translated
asyncfunction of the new writer API. The async version of new writer API is wrapped insideTask, to avoid the "async" syntax being exposed further up the call tree. Mainly because I'm not familiar with the codebase and I worry too many code path changes(i.e. when the completion block argument is getting called, immediately, or in an async way) may cause issues.Test instructions
I think (according to my understanding of the codebase, not the product...), here is a list of area that are changed in this PR. To test, make sure they are working as expected:
Regression Notes
Potential unintended areas of impact
None.
What I did to test those areas of impact (or what existing automated tests I relied on)
N/A
PR submission checklist:
RELEASE-NOTES.txtif necessary.