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

Fix profile access on ProfileStore #1384

Merged
merged 1 commit into from
Nov 14, 2024
Merged

Fix profile access on ProfileStore #1384

merged 1 commit into from
Nov 14, 2024

Conversation

GhenadieVP
Copy link
Contributor

@GhenadieVP GhenadieVP commented Nov 12, 2024

Fixed the profile access on Profile store to avoid crashing the app if some parent task was cancelled.

@@ -23,16 +23,11 @@ final actor ProfileStore {
}

extension ProfileStore {
func profile() async -> Profile {
try! await profileSubject.first()
Copy link
Contributor Author

@GhenadieVP GhenadieVP Nov 12, 2024

Choose a reason for hiding this comment

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

This implementation would crash the app if parent task was cancelled, as here a force unwrap would be done when a CancellationError is thrown. Instead, profileSubject is transformed to AsyncCurrentValueSubject<Profile?> to be able to access the current profile in non-async context.

Copy link
Contributor

Choose a reason for hiding this comment

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

btw I wonder if our init should be updated, we do:

	private init() {
		Task {
			for try await state in await ProfileStateChangeEventPublisher.shared.eventStream() {
				if case let .loaded(profile) = state {
					self.profileSubject.send(profile)
				}

				self.profileStateSubject.send(state)
			}
		}
	}

I would think we would wanna do 2 changes:

  1. Assign the task to a variable
  2. use [weak self] to not have retain cycle

I think 1. is not "needed" because we have a retain cycle, which keeps the Task around,
I think 2. is not "needed" because this is essentially a singleton we need for the duration of the app life time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't do this in the first place exactly for the reasons you describe, as we don't gain anything. Storing in variable might be slightly confusing, as one would do that, if they want to potentially cancel the task at some point.

Copy link
Contributor

@CyonAlexRDX CyonAlexRDX left a comment

Choose a reason for hiding this comment

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

LGTM

@GhenadieVP GhenadieVP merged commit 865721a into main Nov 14, 2024
6 checks passed
@GhenadieVP GhenadieVP deleted the profile_fix branch November 14, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants