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

Improve transition to/from "offline" state #12196

Conversation

kenneth-marut-work
Copy link
Contributor

@kenneth-marut-work kenneth-marut-work commented Feb 17, 2023

What it does

Fixes #12194 by adding a timeout to ConnectionStatusService's ping service to make sure frontend is aware of network interruptions sooner if backend is hanging.

Fixes #12195 by cancelling save/sync in monaco-editor-model when ConnectionStatusService's onStatusChanged event fires preventing scheduled saves/syncs from never resolving when backend goes offline

The hang-backend package and command will be removed before merging this PR.

Breaking Changes
Modifies constructor args for MonacoEditorModel to pass in ConnectionStatusService

How to test

To test #12194:

  1. Build this branch
  2. Start Theia in browser or electron mode
  3. run command "hang backend" to kick off a 30second backend loop
  4. After 10s, a connection interruption should be detected by the frontend (2 * the usual ping timeout)
  5. After the 30 second loop completes, the frontend will recover
  6. Compare with the usual bad behavior described here

To test #12195:

  1. Open an editor under normal conditions
  2. make some changes to that file and try to save them, observe normal behavior
  3. Now execute "hang backend" command
  4. Try to save that file, observe that it doesn't work
  5. Frontend will recognize interruption after 10s, observe that saving still does not work
  6. After 30 seconds, frontend will recover, observe that saving does work now
  7. Compare with bad behavior described here

Review checklist

Reminder for reviewers

@kenneth-marut-work kenneth-marut-work changed the title Draft: Improve transition to/from "offline" state Improve transition to/from "offline" state Feb 17, 2023
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

This fixes the clearest manifestation of the problem, but I wonder if the fix is in the right place. See my comment on the changes to the monaco-editor-model file.

@@ -90,6 +91,7 @@ export class MonacoEditorModel implements IResolvedTextEditorModel, TextEditorDo
protected readonly resource: Resource,
protected readonly m2p: MonacoToProtocolConverter,
protected readonly p2m: ProtocolToMonacoConverter,
protected readonly connectionStatusService: ConnectionStatusService,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should get a 'breaking changes' mention.

Comment on lines +110 to +113
this.toDispose.push(this.connectionStatusService.onStatusChange(() => {
this.cancelSave();
this.cancelSync();
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

The this is where the problem manifested itself, but I wonder if the the problem shouldn't be fixed in the FileResource code, since that's where the saving happens, and it's the connection to the backend file system that breaks saving / loading new content?

@vince-fugnitto
Copy link
Member

I believe we can close the pull-request for the time being, and revisit as necessary.
Both Kenneth and Colin are busy with other projects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
3 participants