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

GH-8723: From now on, proxy factories do not wait for reconnect once the websocket connection was disposed #8803

Conversation

kittaakos
Copy link
Contributor

What it does

It changes the behavior of the proxy-factories when running the app in an electron environment. As logged under #8723, there is no promise rejection when the user tries to save the content of the dirty editors and the backend process is down.
With the current logic from the master, as soon as the websocket connection is disposed, all remote calls are queued up and wait until there is another, new websocket connection. This behavior can be useful in the browser environment, but not necessarily required for Electron. There is not much we can do other than handling the promise rejection and notifying the user about the fatal crash.

This PR is a noop for the browser.

If you have a better idea of how to make this customization without if (!environment.electron.is()), let me know. I want to keep the changeset as small and possible. Of course, this PR must not introduce any breaking change in downstream.

How to test

  • Build the electron app with the sample,
  • Start the elecrton app,
  • Disable auto-save,
  • Make one of the files dirty,
  • Kill the backend process (you can search for Backend is up and running. [PID: number] in the logs),
  • Save the editor.
  • You should see the handled rejection:

Screen Shot 2020-11-30 at 16 49 17

Closes #8723

Review checklist

Reminder for reviewers

Akos Kitta added 2 commits November 30, 2020 16:40
When running in electron, the proxy factory should reject all remote
calls if the websocket connection was closed abnormally (`1006`) .

Closes eclipse-theia#8723

Signed-off-by: Akos Kitta <kittaakos@typefox.io>
Signed-off-by: Akos Kitta <kittaakos@typefox.io>
@vince-fugnitto vince-fugnitto added core issues related to the core of the application electron issues related to the electron target labels Nov 30, 2020
paul-marechal
paul-marechal previously approved these changes Nov 30, 2020
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

LGTM, confirmed that the issue is fixed on Windows 10.

@@ -329,6 +329,7 @@ export class MonacoEditorModel implements ITextEditorModel, TextEditorDocument {
await operation();
} catch (e) {
console.error(e);
throw e;
Copy link
Member

Choose a reason for hiding this comment

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

it does not throw on purpose to avoid rejecting consequent operations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the note. Well, we should reject it somehow. That would be the desired flow when clients want to handle errors. If the error was swallowed on purpose, that's a problem. What was the problematic use-case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can filter the error and rethrow only when it indicates t a connection disposed issue, but the rejection must happen, preferably always.

Copy link
Member

@akosyakov akosyakov Dec 1, 2020

Choose a reason for hiding this comment

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

maybe something like:

const result = this.pendingOperation.then(() = operation());
this.pendingOperation = result.catch(e => console.error(e));
return result;

but we should also check all clients of run that they expect that now sync and save can throw and can deal with it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will check the call sites. Thanks! Eventually, I am fine with introducing a new template method, handleError, with console.log(error) default implementation in the monaco-editor-model.ts module, but best would be to have the error handling in Theia.

@JonasHelming
Copy link
Contributor

@paul-marechal : Could you decide whether we want to merge this?

@paul-marechal
Copy link
Member

I am not confident merging this patch after all this time. There's also unresolved comments.

I'd rather close this one for now and keep track of the issue through #10723

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core issues related to the core of the application electron issues related to the electron target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Application shell's save all does not reject when save has failed
5 participants