Skip to content

fix(apple): Ensure resources are updated on MainActor#8064

Merged
jamilbk merged 1 commit into
mainfrom
fix/resources-main
Feb 9, 2025
Merged

fix(apple): Ensure resources are updated on MainActor#8064
jamilbk merged 1 commit into
mainfrom
fix/resources-main

Conversation

@jamilbk

@jamilbk jamilbk commented Feb 9, 2025

Copy link
Copy Markdown
Member

This fixes a bug introduced in db655dd that could lead to a crash or undefined behavior because it potentially updates resources (a @Published object) from a background thread.

UI updates must occur on the main thread only.

@vercel

vercel Bot commented Feb 9, 2025

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
firezone ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 9, 2025 4:11pm

@thomaseizinger thomaseizinger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems very brittle. Is there no better way to enforce this?

@jamilbk

jamilbk commented Feb 9, 2025

Copy link
Copy Markdown
Member Author

This seems very brittle. Is there no better way to enforce this?

The compiler usually catches these. Unfortunately in this case it didn't, presumably because it's a callback.

We did receive a runtime warning though in the logs, which is how I caught it.

@jamilbk
jamilbk added this pull request to the merge queue Feb 9, 2025
@thomaseizinger

Copy link
Copy Markdown
Member

The compiler usually catches these. Unfortunately in this case it didn't, presumably because it's a callback.

Can we establish a pattern where access to these fields always need to go through annotated functions or something like that?

Merged via the queue into main with commit ce81dc5 Feb 9, 2025
@jamilbk
jamilbk deleted the fix/resources-main branch February 9, 2025 23:08
@jamilbk

jamilbk commented Feb 9, 2025

Copy link
Copy Markdown
Member Author

The compiler usually catches these. Unfortunately in this case it didn't, presumably because it's a callback.

Can we establish a pattern where access to these fields always need to go through annotated functions or something like that?

Hm yeah, I think we can actually mark this callback closure @MainActor and the compiler will warn us. I'll do that in a PR.

github-merge-queue Bot pushed a commit that referenced this pull request Feb 10, 2025
Followup to the discussion on
#8064. By annotating the
callback that updates our Resources `@MainActor`, the compiler will
correctly warn us when we call it from a non-isolated context.
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.

2 participants