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

Offline check only occurs on General page #2972

Closed
gunamata opened this issue Sep 21, 2022 · 8 comments · Fixed by #2988
Closed

Offline check only occurs on General page #2972

gunamata opened this issue Sep 21, 2022 · 8 comments · Fixed by #2988
Assignees
Labels
area/diagnostics kind/bug Something isn't working
Milestone

Comments

@gunamata
Copy link
Contributor

After Rerun, the Diagnostics page should reflect any changes happened in the background. For example,

In a running session,

  • Go to Diagnostics tab
  • Disconnect from network
  • Click the Rerun button

The Diagnostics page should a line item about the offline status.

The change reflects when I switch to some other tab, say General and then come back to Diagnostics tab.

@gunamata gunamata added the kind/bug Something isn't working label Sep 21, 2022
@gunamata gunamata added this to the Next milestone Sep 21, 2022
@gunamata
Copy link
Contributor Author

Some more observations about the behavior,

The change reflects only if I switch to General tab and come back to Diagnostics.. If I switch to some other tab say Port Forwarding, and come back to Diagnostics the change doesn't reflect.

@mook-as mook-as changed the title After Rerun, the Diagnostics page should reflect any changes happened in the background Offline check only occurs on General page Sep 21, 2022
@ericpromislow
Copy link
Contributor

This might be related to the auth-failure errors we're seeing.

With the docker-plugin symlink failure, I always had to press the Rerun button to see an update.

And on macos, the network change was observable when going to any other tab and back to Diagnostics,
not just General.

@gunamata
Copy link
Contributor Author

One more observation, the Kube context check immediately reflects on Rerun though!

@ericpromislow ericpromislow removed their assignment Sep 22, 2022
@jandubois
Copy link
Member

We'll have to retest this later when we have a build with all the fixes from today.

I want you (@gunamata) and @prabalsharma to check your diagnostics.log if you have any trapped errors that we ignored. That would (maybe) explain why the network didn't update when the status changed, and you had to switch tabs.

But it is hard to keep all the fixes from today separate in my brain, so I would rather retest with a fresh main build.

@jandubois
Copy link
Member

I can repro this on macOS too. The network check only updates the badge icon when I move away from the diagnostics tab, and then back to it. However, I did not have to switch to the General tab, the other tabs worked as well.

This may be related to #2976, which looks like the badge count is only updated when you switch to the Diagnostics tab (or when you press Re-run).

@IsaSih
Copy link
Contributor

IsaSih commented Sep 22, 2022

Tried to repro that on macOS but it worked fine for me. After clicking rerun, the diagnostics table was updated, adding a network warning message. Once I reconnected to the network, I clicked rerun again, and the warning disappeared with no need to change tabs.

@adamkpickering
Copy link
Member

The cause of this problem is that network state is only updated when we have the General tab loaded. This is because we use window.navigator.onLine and the online and offline events of window. I found this quite surprising - I expected checks for network connectivity to come from the backend, not the frontend. It isn't surprising that we ran into this issue given how all of this works. It's convoluted.

Even stranger, it seems that the result from the frontend isn't always used. In src/main/tray.ts:Tray.handleUpdateNetworkStatus, the result from the frontend is taken if it is truthy, and the result of checkConnectivity is taken otherwise.

But it gets one step weirder. In my testing, checkConnectivity (or rather, dns.lookup, which it calls) is not reliable in determining whether there is network connectivity. It keeps returning a valid result long after network connectivity is lost.

Another thing: why do we still have network status in the General tab when we have the network connectivity diagnostic?

Here's what I think we should do:

  1. Forget about getting network connectivity status from window.navigator.onLine and the online and offline events on window. Instead, have either a polling worker, or just a function, that can check for network connectivity from the backend. Use this in the renderer through either events or the CLI server. This way we have a single source of truth on network connectivity that can be used in the frontend or backend.
  2. Remove the network status in the General tab in favour of the network connectivity diagnostic.

@adamkpickering
Copy link
Member

Created branch 2974-fix-network-status with some progress on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment