Skip to content

@MainActor annotations#42

Merged
lickel merged 19 commits into
feature/5.0from
mainactor
Oct 17, 2022
Merged

@MainActor annotations#42
lickel merged 19 commits into
feature/5.0from
mainactor

Conversation

@lickel

@lickel lickel commented Oct 13, 2022

Copy link
Copy Markdown
Collaborator
  • 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

codecov-commenter commented Oct 13, 2022

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Comment thread 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

Copy link
Copy Markdown

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
Copy Markdown
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.

3 participants