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

Web socket disconnection: file explorer progress bar issue #13216

Closed
safisa opened this issue Dec 28, 2023 · 6 comments
Closed

Web socket disconnection: file explorer progress bar issue #13216

safisa opened this issue Dec 28, 2023 · 6 comments
Milestone

Comments

@safisa
Copy link
Contributor

safisa commented Dec 28, 2023

Bug Description:

Hi,

In the latest Theia 1.45, with the new web socket management and the front-end connection timeout, once I have a disconnection and try to expand a folder in the file explorer, the spinner on the folder keeps going also the progress bar in the file explorer keeps visible (although after getting connected back within the timeout period).

See the attached screenshot:

image

Thanks

Steps to Reproduce:

  1. set the frontendConnectionTimeout to some positive value and the reloadOnReconnect to false
  2. build and start Theia browser
  3. do a network disconnection
  4. exand any folder in the file explorer
  5. reconnect back
  6. see thath the spinner icon and the progress bar still visible

Thanks

Additional Information

  • Operating System: Mac
  • Theia Version: Theia 1.45
@JonasHelming
Copy link
Contributor

@tsmaeder

@safisa
Copy link
Contributor Author

safisa commented Jan 3, 2024

I think the issue is in the "refresh" method of the TreeImpl class (packages/core/src/browser/tree/tree.ts).
The resolveChildren is failing (no connection), and maybe the CancellationToken is not working in this case to reset the busy status...

@tsmaeder
Copy link
Contributor

tsmaeder commented Jan 4, 2024

Here's a scenario which I think could explain this:

  1. refresh() calls doMarkBusy()
  2. doMarkBusy() waits for a 800ms timeout
  3. refresh() calls resolveChildren() which fails immediately.
  4. busySource.cancel() is invoked
  5. doMarkBusy() timeout expired, tree is marked as busy.
  6. doResetBusy() is never invoked, because the token was canceled before the listener was added.

I think the fix would be to check token.isCancellationRequested before calling doSetBusy() in doMakrBusy().

@safisa would you have time to give this a try?

@safisa
Copy link
Contributor Author

safisa commented Jan 4, 2024

Hi @tsmaeder

This fix is not enough, I had to move the onCancellationRequested to be before the timeout:

protected async doMarkAsBusy(node: Mutable<TreeNode>, ms: number, token: CancellationToken): Promise<void> {
      try {
          token.onCancellationRequested(() => this.doResetBusy(node));
          await timeout(ms, token);
          if (token.isCancellationRequested) { return; }
          this.doSetBusy(node);
      } catch {
          /* no-op */
      }
  }

After this change, the problem is resolved, the user needs to expand the node again after the connection is back online, and then the node's busy state is reset (if we don't try to expand it, it will stay in a busy state, but I think this is the wanted behavior).

@safisa
Copy link
Contributor Author

safisa commented Jan 9, 2024

Hi @tsmaeder

If the code change above is good enough, do you want me to apply a PR? or you are doing it?

@tsmaeder
Copy link
Contributor

@safisa please open a PR.

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

No branches or pull requests

4 participants