Skip to content

fix(gui-client): defer GUI exit until tunnel closes#6874

Merged
ReactorScram merged 5 commits into
mainfrom
fix/sentry-shutdown-issue
Oct 1, 2024
Merged

fix(gui-client): defer GUI exit until tunnel closes#6874
ReactorScram merged 5 commits into
mainfrom
fix/sentry-shutdown-issue

Conversation

@ReactorScram
Copy link
Copy Markdown
Contributor

@ReactorScram ReactorScram commented Sep 30, 2024

Closes #6873

The issue seems to be a race between flushing Sentry in the GUI process and shutting down Firezone in the tunnel daemon (IPC service).

With this change, the GUI waits to hear DisconnectedGracefully from the tunnel daemon before flushing Sentry, and the issue is prevented.

Adding the new state and new IPC message required small changes in several places

…n on Windows

Closes #6873

```[tasklist]
- [ ] Minimize diff
- [ ] Update changelog
```
@ReactorScram ReactorScram self-assigned this Sep 30, 2024
@vercel
Copy link
Copy Markdown

vercel Bot commented Sep 30, 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 Oct 1, 2024 3:43pm

@ReactorScram
Copy link
Copy Markdown
Contributor Author

Is it worth adding testing around Controller here? This is sort of a weird corner case... If we tested Controller without running the tunnel, it would not have triggered

@ReactorScram ReactorScram marked this pull request as ready for review September 30, 2024 21:49
Comment on lines +147 to +149
TunnelReady {
resources: Vec<ResourceDescription>,
},
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure why cargo fmt changed its mind on this

}

/// True if we should react to `OnUpdateResources`
fn needs_resource_updates(&self) -> bool {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Unrelated refactor but wasn't quite big enough to split out

Comment on lines +29 to +31
<ChangeItem enable={title === "Windows"} pull="6874">
Fixes a delay when closing the GUI
</ChangeItem>
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I couldn't replicate it on the Headless Client, this is explained by the Headless Client not having the IPC boundary, so it's just one thread and doesn't race between shutting down connlib and shutting down Sentry.

All I can think for the Linux GUI Client is that Linux shuts down a lot quicker, so there is a race but it never happened to trigger for me.

@thomaseizinger thomaseizinger changed the title fix(rust/gui-client/windows): Fixes Sentry flushing when shutting down fix(gui-client): defer GUI quit until IPC service is stopped Oct 1, 2024
@thomaseizinger thomaseizinger changed the title fix(gui-client): defer GUI quit until IPC service is stopped fix(gui-client): defer GUI exit until IPC service is stopped Oct 1, 2024
@thomaseizinger
Copy link
Copy Markdown
Member

I slightly reworded the title, hope you don't mind :)

Comment thread rust/gui-client/src-common/src/controller.rs Outdated
return self
.handle_connect_result(result)
.await
.map(|_| ControlFlow::Continue(()))
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.

Would prefer use of ?.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tried a couple different things, including return Ok(ControlFlow::from(self.handle_connect_result(result).await?));, and none of them compiled?

Comment thread rust/headless-client/src/main.rs Outdated
Comment thread website/src/components/Changelog/GUI.tsx Outdated
Comment thread rust/gui-client/src-common/src/controller.rs Outdated
@ReactorScram ReactorScram changed the title fix(gui-client): defer GUI exit until IPC service is stopped fix(gui-client): defer GUI exit until connlib is stopped Oct 1, 2024
@ReactorScram
Copy link
Copy Markdown
Contributor Author

I reworded it again, we aren't actually waiting for the IPC service itself to stop, we're waiting for the tunnel to close down

ReactorScram and others added 2 commits October 1, 2024 10:05
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
@ReactorScram ReactorScram changed the title fix(gui-client): defer GUI exit until connlib is stopped fix(gui-client): defer GUI exit until tunnel closes Oct 1, 2024
@ReactorScram ReactorScram enabled auto-merge October 1, 2024 15:43
@ReactorScram ReactorScram added this pull request to the merge queue Oct 1, 2024
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 1, 2024
@ReactorScram ReactorScram added this pull request to the merge queue Oct 1, 2024
Merged via the queue into main with commit 05acdd5 Oct 1, 2024
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.

bug(rust/gui-client/windows): delay when quitting related to Sentry

2 participants