Skip to content

fix(apple): Load tunnel manager after creating it#7593

Merged
jamilbk merged 4 commits intomainfrom
fix/load-tunnel-manager
Dec 30, 2024
Merged

fix(apple): Load tunnel manager after creating it#7593
jamilbk merged 4 commits intomainfrom
fix/load-tunnel-manager

Conversation

@jamilbk
Copy link
Member

@jamilbk jamilbk commented Dec 29, 2024

When launching Firezone for the first time, the VPN profile doesn't exist. We prompt the user to create it with "Grant VPN Permission", but then we fail to reload it, which initializes the tunnelManager instance variables properly and binds observers.

The result of this was that we failed to react to VPN status changes on the first launch of Firezone.

This can (and should be) refactored to be cleaner, but that is out of scope for this PR and will be saved for #6554.

Refs #7579

@vercel
Copy link

vercel bot commented Dec 29, 2024

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 Dec 30, 2024 6:56pm

DispatchQueue.main.async { [weak self] in
guard let self else { return }

Task {
Copy link
Member

Choose a reason for hiding this comment

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

I don't know Swift very well. What is the point of spawning a task inside a dispatch queue? Those sound like equivalent things?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. DispatchQueue.main.async takes a synchronous function as its argument, so we need to wrap the call to TunnelManager.shared.create() with a Task.

However, I think it makes more sense to flip these around like so:

  func createVPNProfile() {
    Task {
      try await TunnelManager.shared.create()

      DispatchQueue.main.async { [weak self] in
        guard let self else { return }
        
        // Load the new settings and bind observers
        self.loadTunnelManager()
      }
    }
  }

Copy link
Member

Choose a reason for hiding this comment

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

Task itself spawns, right? In other words, the task starts executing as soon as the task is created. So it seems redundant here?

Copy link
Member Author

Choose a reason for hiding this comment

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

They serve different purposes -- Task creates an async context which starts executing immediately, but without any guarantees on which thread.

DispatchQueue.main queues the function to execute asynchronously on the main thread, where it's required to be for UI updates. Because the self.loadTunnelManager function updates the UI and binds the tunnel manager updates to the MenuBar / UI views, that needs to happen on the main thread.

Copy link
Member Author

Choose a reason for hiding this comment

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

And since the function passed to DispatchQueue.main.async must be synchronous, we can't call try await TunnelManager.shared.create() directly in that block.

Hmm, actually, since this is the only place where that async function is called, maybe I can just make it blocking. Let me see.

@jamilbk jamilbk added this pull request to the merge queue Dec 30, 2024
auto-merge was automatically disabled December 30, 2024 19:16

Pull Request is not mergeable

Merged via the queue into main with commit a1337d0 Dec 30, 2024
@jamilbk jamilbk deleted the fix/load-tunnel-manager branch December 30, 2024 19:23
github-merge-queue bot pushed a commit that referenced this pull request Dec 30, 2024
…#7594)

In certain weird edge cases such as:

- The user removes the VPN profile while Firezone is signed in, causing
the `NETunnelProviderSession` to go invalid immediately
- The user approves the system extension to load before the VPN profile
access is granted

then the TunnelManager will not be able to obtain a valid reference to a
NETunnelProviderSession object.

In these cases, for now, it makes more sense to fail silently than to
crash, effectively making these operations a no-op until the user
remedies the VPN profile. Currently the user is prompted to re-grant VPN
profile whenever its status goes to `invalid`, so these cases don't
technically fail without prompting the user.

Draft because it's stacked on #7593 


Fixes #7579 
Fixes #7591

---------

Signed-off-by: Jamil <jamilbk@users.noreply.github.com>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
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