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

Create WeakResolver #97

Merged
merged 1 commit into from
Dec 6, 2023
Merged

Create WeakResolver #97

merged 1 commit into from
Dec 6, 2023

Conversation

skorulis-ap
Copy link
Collaborator

@skorulis-ap skorulis-ap commented Dec 6, 2023

Background

It is expected that parts of the dependency graph will directly hold onto a Resolver as they need to resolve services after initialisation. (e.g. pushing a new view controller). This creates the potential that if the object holding the Resolver leaks then so does the Resolver. While this can be detected, it is difficult to pinpoint the cause of the issue without the aid of debug tooling.

Solution

This PR adds the concept of a WeakResolver which holds a weak reference to the underlying Container which works as a Resolver until the Container has been deallocated at which point any attempt to resolve a service will cause a crash.

@skorulis-ap skorulis-ap requested a review from a team December 6, 2023 02:47
@skorulis-ap skorulis-ap marked this pull request as ready for review December 6, 2023 03:00
Sources/KnitLib/WeakResolver.swift Outdated Show resolved Hide resolved
public var isAvailable: Bool { return container != nil }

public var strongResolver: WeakAwareResolver { unwrapped }
public var optionalResolver: WeakAwareResolver? { container }

Choose a reason for hiding this comment

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

What are these properties used for?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is to allow for specific use cases to access the underlying container if they are willing to take responsibility of deallocating correctly. Example would be some short-lived service on log out. I don't think it will likely be needed but it is good to have in case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding some documentation to both properties to make explicit the responsibility of the consumer to correctly manage life cycle would be good

Copy link
Collaborator Author

@skorulis-ap skorulis-ap Dec 6, 2023

Choose a reason for hiding this comment

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

isAvailable: Can I do the thing?
optionalResolver: Do the thing if possible
strongResolver: The thing has to happen, I'm aware of the risks (now removed)

Copy link
Collaborator

@bradfol bradfol left a comment

Choose a reason for hiding this comment

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

Looks great!

I think we should consider removing the strongResolver property however

Sources/KnitLib/WeakResolver.swift Outdated Show resolved Hide resolved
Tests/KnitLibTests/WeakResolverTests.swift Outdated Show resolved Hide resolved
@skorulis-ap skorulis-ap force-pushed the skorulis/weak-resolver branch from d6244ee to 864db8a Compare December 6, 2023 22:19
@skorulis-ap skorulis-ap merged commit d814f59 into main Dec 6, 2023
1 check passed
@skorulis-ap skorulis-ap deleted the skorulis/weak-resolver branch December 6, 2023 22:48
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