Skip to content

Conversation

@lickel
Copy link
Collaborator

@lickel lickel commented Oct 13, 2022

  • Annotate many methods as @mainactor
    • All delegate methods
    • All code with assert(Thread.isMainThread)
  • Faulting an association when you're off the main thread will have different characteristics
    • If the association already exists, nothing will change
    • If the association does not already exit, it will always return nil and hit the main thread to batch fetch the associations

@lickel lickel marked this pull request as draft October 13, 2022 17:49
@lickel lickel marked this pull request as ready for review October 13, 2022 18:13
@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2022

Codecov Report

Base: 93.90% // Head: 93.44% // Decreases project coverage by -0.45% ⚠️

Coverage data is based on head (2985eaa) compared to base (9cb0fb0).
Patch coverage: 72.25% of modified lines in pull request are covered.

Additional details and impacted files
@@               Coverage Diff               @@
##           feature/5.0      #42      +/-   ##
===============================================
- Coverage        93.90%   93.44%   -0.46%     
===============================================
  Files               30       30              
  Lines             4958     5038      +80     
===============================================
+ Hits              4656     4708      +52     
- Misses             302      330      +28     
Impacted Files Coverage Δ
...Sources/Associations/FetchRequestAssociation.swift 86.93% <ø> (ø)
.../Controller/FetchedResultsControllerProtocol.swift 90.10% <ø> (ø)
FetchRequests/Sources/FetchableObject.swift 100.00% <ø> (ø)
...tchRequests/Sources/Requests/FetchDefinition.swift 90.90% <ø> (ø)
...tchRequests/Sources/SwiftUI/FetchableRequest.swift 91.04% <ø> (ø)
...ibleSectionsFetchedResultsControllerTestCase.swift 99.51% <ø> (ø)
...Controllers/FetchedResultsControllerTestCase.swift 100.00% <ø> (ø)
...trollers/FetchedResultsControllerTestHarness.swift 100.00% <ø> (ø)
...Tests/Models/FetchRequestAssociationTestCase.swift 98.86% <ø> (ø)
...uests/Tests/SwiftUI/FetchableRequestTestCase.swift 100.00% <ø> (ø)
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

SKIP_INSTALL = YES;
SWIFT_ACTIVE_COMPILATION_CONDITIONS = DEBUG;
SWIFT_OPTIMIZATION_LEVEL = "-Onone";
SWIFT_STRICT_CONCURRENCY = targeted;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This runs "clean".
Full concurrency checking would fail in many ways (notably @MainActor(unsafe) does not work).


internal class FetchRequestObservableToken<Parameter>: ObservableToken {
private let _observe: (_ handler: @escaping (Parameter) -> Void) -> Void
typealias Handler = (Parameter) -> Void
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Making this @mainactor (which requires making ObservableToken's handler @mainactor) was a giant pain and not worth it since I could do the simple main-thread bouncing behavior.


guard let index = fetchedObjects.firstIndex(where: { $0.id == objectID }) else {
if !Thread.isMainThread {
DispatchQueue.main.async { [weak self] in
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

performOnMainThread would need to support throwing in an awkward way if I wanted to reuse it here.


let handleSort: (FetchedObject, Bool, Any?, Any?) -> Void = { [weak self] object, isSection, old, new in
let handleSort: @MainActor (FetchedObject, Bool, Any?, Any?) -> Void = { [weak self] object, isSection, old, new in
assert(Thread.isMainThread)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This always required main thread behavior


let handleChange: (FetchedObject) -> Void = { [weak self] object in
let handleChange: @MainActor (FetchedObject) -> Void = { [weak self] object in
assert(Thread.isMainThread)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This always required main thread behavior


memoryPressureToken?.observeIfNeeded { [weak self] notification in
self?.removeAllAssociatedValues()
performOnMainThread {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New behavior: support for bouncing to main for these handful of observations

CHANGELOG.md Outdated
* All code with assert(Thread.isMainThread)
* Faulting an association when you're off the main thread will have different characteristics
* If the association already exists, nothing will change
* If the association does not already exit, it will always return nil and hit the main thread to batch fetch the associations

Choose a reason for hiding this comment

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

exist and not exit here, I think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hah! yup, fixed

@lickel lickel merged commit ef68980 into feature/5.0 Oct 17, 2022
@lickel lickel deleted the mainactor branch October 17, 2022 20: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.

4 participants