Skip to content

fix(gui-client/linux): show a specific error message in the GUI when updating#5848

Merged
ReactorScram merged 49 commits intomainfrom
fix/ipc-service-stop-message
Jul 15, 2024
Merged

fix(gui-client/linux): show a specific error message in the GUI when updating#5848
ReactorScram merged 49 commits intomainfrom
fix/ipc-service-stop-message

Conversation

@ReactorScram
Copy link
Copy Markdown
Contributor

@ReactorScram ReactorScram commented Jul 11, 2024

Closes #5790 (we could do more, but this might be sufficient)

image image

The code is cross-platform, but this is unlikely to happen on Windows because the MSI refuses to update if the GUI process is running. On Linux apt-get will update and restart the IPC service without touching the GUI process.

- [x] Test on Linux with `apt-get install`
- [x] Update changelog
- [x] Run a 5-minute smoke test on Linux
- [x] Run a 5-minute smoke test on Windows
- [x] Open for review
- [ ] Merge

…updating

Closes #5790 (we could do more, but this might be sufficient)

The code is cross-platform, but this is unlikely to happen on Windows because
the MSI refuses to update if the GUI process is running. On Linux `apt-get`
will update and restart the IPC service without touching the GUI process.
@ReactorScram ReactorScram self-assigned this Jul 11, 2024
@vercel
Copy link
Copy Markdown

vercel bot commented Jul 11, 2024

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

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
firezone ⬜️ Ignored (Inspect) Visit Preview Jul 15, 2024 5:18pm

@ReactorScram ReactorScram changed the base branch from main to feat/tauri-set-icon July 11, 2024 20:56
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jul 11, 2024

Terraform Cloud Plan Output

Plan: 15 to add, 23 to change, 15 to destroy.

Terraform Cloud Plan

Base automatically changed from feat/tauri-set-icon to main July 11, 2024 21:04
Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jul 11, 2024

Performance Test Results

TCP

Test Name Received/s Sent/s Retransmits
direct-tcp-client2server 231.9 MiB (-1%) 233.4 MiB (-1%) 252 (+34%)
direct-tcp-server2client 245.9 MiB (-1%) 247.7 MiB (-1%) 394 (+183%)
relayed-tcp-client2server 240.2 MiB (+1%) 241.3 MiB (+1%) 370 (+3%)
relayed-tcp-server2client 246.6 MiB (+4%) 248.1 MiB (+4%) 721 (+29%)

UDP

Test Name Total/s Jitter Lost
direct-udp-client2server 500.0 MiB (+0%) 0.03ms (-27%) 44.63% (+8%)
direct-udp-server2client 500.0 MiB (+0%) 0.02ms (+201%) 21.14% (-10%)
relayed-udp-client2server 500.0 MiB (-0%) 0.03ms (+75%) 53.86% (+0%)
relayed-udp-server2client 500.0 MiB (-0%) 0.10ms (+55%) 34.36% (-10%)

Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
@ReactorScram ReactorScram marked this pull request as ready for review July 12, 2024 20:57
Copy link
Copy Markdown
Member

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Nice work! Left some food for thought :)

loop {
Handler::new(&mut server).await?.run().await;
let mut handler_fut = pin!(Handler::new(&mut server));
let Some(handler) = poll_fn(|cx| {
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.

A lot of nesting going on here. Could this be simpler if we used a select() instead and matched on the Either?

Copy link
Copy Markdown
Contributor Author

@ReactorScram ReactorScram Jul 15, 2024

Choose a reason for hiding this comment

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

It would take fewer lines, I just don't like how the Either::Left((thing_i_want, _future_i_always_drop)) looks

continue;
let event = poll_fn(|cx| {
if let Poll::Ready(()) = signals.poll_recv(cx) {
// `recv` on signals is cancel-safe.
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.

Typically, when you use poll fns, you don't need to worry about cancel safety unless you constructed a future and are carrying it around as a value but then it is obvious anyway that you are dropping data here.

if let Poll::Ready(()) = signals.poll_recv(cx) {
// `recv` on signals is cancel-safe.
Poll::Ready(Event::Terminate)
} else if let Poll::Ready(result) = pin!(&mut self.ipc_rx).poll_next(cx) {
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.

For complex return types from a poll like this, I typically match right away so there is only one layer of pattern matching.

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.

Like this? (Comparing old and new)
image
Maybe if I did use Poll::Ready. Otherwise the lines are a bit long.

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.

Yeah! The lines are long but the benefit is that the compiler makes an exhaustiveness check for you so you can glance over it in most cases.

@ReactorScram ReactorScram enabled auto-merge July 15, 2024 17:18
@ReactorScram ReactorScram added this pull request to the merge queue Jul 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 15, 2024
@ReactorScram ReactorScram added this pull request to the merge queue Jul 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 15, 2024
@ReactorScram ReactorScram added this pull request to the merge queue Jul 15, 2024
Merged via the queue into main with commit b539c01 Jul 15, 2024
@ReactorScram ReactorScram deleted the fix/ipc-service-stop-message branch July 15, 2024 18:05
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(gui-client/linux): Broken pipe error / installing a new deb while the GUI is signed in

2 participants