reload plugins on reconnect #6159

akosyakov · 2019-09-11T07:15:01Z

What it does

Out of scope:

handling extension dependencies on stop
UI to uninstall extensions

How to test

reconnect because of a connection failure
- no reproducible with Chrome since it cannot throttle web socket connections
- throttle with the system proxy instead, for instance with https://www.charlesproxy.com/
reconnect on the server restart
test everything with multiple windows for the same backend

Hints:

Take memory snapshots when a connection is gone to check that there is no main services are left.

Tested with following VS Code extensions:

trees - tree-view-sample
languages - ms-vscode.Go
debugging - node-debug and node-debug2
scm - built-in git
webviews - webview-sample
- make sure to use this commit, never versions use asWebviewUri api
quick pick/input - quickinput-sample

Reconnecting tree views

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

amiramw · 2019-09-19T06:18:07Z

@akosyakov do you think custom APIs should also be treated to allow unload?

akosyakov · 2019-09-19T06:52:27Z

do you think custom APIs should also be treated to allow unload?

@amiramw Custom main services should implement Disposable interface, then they will be disposed. I will add it in the changelog.

akosyakov · 2019-09-19T07:11:52Z

@eclipse-theia/eclipse-theia I'm still trying different extensions to check reload of all contributions, but the code should be in final state and help with testing is welcomed!

svenefftinge

The code looks great. Will now test.

packages/monaco/src/browser/textmate/textmate-registry.ts

svenefftinge · 2019-09-19T12:20:05Z

packages/plugin-ext/src/common/rpc-protocol.ts

+        }
+        this.locals.set(identifier.id, instance);
+        if (Disposable.is(instance)) {
+            this.toDispose.push(instance);


It is a little strange that the dispose lifecycle of the passed in arguments get associated with this object. I see that the usages always pass fresh objects here. So maybe not a real problem.
In general we should follow the pattern 'who creates is responsible for disposing'

i actually thought that set should be replaced by a contribution point, something like PluginRPCService, then contributions will be created by RPCProtocol and installed, but first we need to refactor the plugin system to use DI.

packages/plugin-ext/src/main/browser/connection-main.ts

packages/plugin-ext/src/main/browser/plugin-ext-deploy-command.ts

packages/plugin-ext/src/main/browser/text-editors-main.ts

packages/plugin-ext/src/main/browser/plugin-shared-style.ts

packages/plugin-ext/src/plugin/plugin-manager.ts

svenefftinge · 2019-09-19T14:52:50Z

Works great as well!

I noticed that after disconnecting and reconnecting again, there is an additional reconnection.
I.e. I see

root INFO Registering scopes for language: java.
logger-protocol.ts:112 root INFO The scopes have been successfully registered for java.
index.js:94 Error: Connection timeout
    at index.js:85
(anonymous) @ index.js:94
setTimeout (async)
emitError @ index.js:84
(anonymous) @ index.js:127
setTimeout (async)
connect @ index.js:124
setTimeout (async)
handleClose @ index.js:113
logger-protocol.ts:112 root INFO Unregistering scopes for language: java.
logger-protocol.ts:112 root INFO The scopes have been successfully unregistered for java.

but I think you mentioned that once to me and it is also unrelated to this PR as it happens without the change, too. With this PR this at least doesn't have such a bad side effect anymore. 🎉

Also: - fix #5829: ensure that each plugin is loaded only once - fix #6186: start only one plugin host process for the same host instead of a new for each load as before - revert #5257: Reconnect same host plugin process and client" 797db75 Signed-off-by: Anton Kosiakov <anton.kosyakov@typefox.io>

akosyakov · 2019-09-20T06:20:27Z

merging if the build is green

benoitf · 2019-09-20T13:07:46Z

packages/plugin-ext/src/common/plugin-api-rpc.ts

@@ -163,7 +163,7 @@ export const emptyPlugin: Plugin = {
 };

 export interface PluginManagerExt {
-    $stopPlugin(contextPath: string): PromiseLike<void>;
+    $stop(pluginId?: string): PromiseLike<void>;


why break the name of this method ?

Because it does not do what it means: It does not stop one plugin but all plugins. contextPath is unused, changing contextPath to optional pluginId to stop either one plugin or all was already a breaking change. I could introduce another method, but then stopPlugin won't be used anywhere in the Theia repo. I did not see a reason why to keep an unused method.

benoitf · 2019-09-20T13:08:02Z

packages/plugin-ext/src/hosted/node/plugin-host-rpc.ts

@@ -80,16 +80,6 @@ export class PluginHostRPC {
        }
    }

-    /*


Hello, why stopContext has been removed ? it's also not referenced in changelog ?

plugin-host-rpc is quite sensitive as it's used downstream like Eclipse Che.

Also I don't see it related to the changes of this PR

There were a clean up of plugin unloading that they can be uninstalled and unused clients of refactored code were removed. Code which should be used in a downstream project should be moved to a corresponding project, not kept in the Theia repo without any internal clients and ability to test by any committer.

There is a record that this PR breaks the plugin system. So far we were not listing individual changes only notified that some part will be broken in next release by some PRs, since we did not really document anything as APIs. Please raise it at the dev meeting if you suggest that each a change to each function should be listed.

Please review a PR while it is not merged, not afterwards. It was 9 days while it was in draft mode for the design review. Committers are not expected to review and test PRs against downstream projects or wait when someone does it.

Please also opened follow-up issues to already merged PRs. Not many committers read comments on them and it can get lost.

If you're considering that removing code or changing structural behavior is only requiring one approval from a committer of the same company, I think something is wrong.

Please add to discuss to the dev meeting.

Regarding to

Code which should be used in a downstream project should be moved to a corresponding project, not kept in the Theia repo without any internal clients and ability to test by any committer.

I remembered that we don't have something like DI in the plugin host process yet, not sure whether you can actually extend it. If not please let me know, the new issue will be the best.

akosyakov added the plug-in system issues related to the plug-in system label Sep 11, 2019

akosyakov force-pushed the ak/plugins_reconnect branch 29 times, most recently from 10f41f1 to 2a653b5 Compare September 16, 2019 05:27

akosyakov force-pushed the ak/plugins_reconnect branch 5 times, most recently from 9ef894e to e8d8233 Compare September 19, 2019 04:26

akosyakov force-pushed the ak/plugins_reconnect branch 2 times, most recently from ce9e771 to 85be868 Compare September 19, 2019 06:51

akosyakov marked this pull request as ready for review September 19, 2019 07:10

akosyakov requested review from svenefftinge and a team September 19, 2019 07:12

akosyakov force-pushed the ak/plugins_reconnect branch 2 times, most recently from 0f57270 to bfe4bb5 Compare September 19, 2019 09:19

akosyakov mentioned this pull request Sep 19, 2019

Question - how Debug server should be deployed to kubernetes cluster ? #4898

Closed

vince-fugnitto mentioned this pull request Sep 19, 2019

http file miss highlight and codelens not effect #6192

Closed

svenefftinge approved these changes Sep 19, 2019

View reviewed changes

akosyakov force-pushed the ak/plugins_reconnect branch from bfe4bb5 to c980100 Compare September 20, 2019 02:48

akosyakov force-pushed the ak/plugins_reconnect branch from c980100 to 50beaee Compare September 20, 2019 06:18

akosyakov merged commit 657bda8 into master Sep 20, 2019

akosyakov deleted the ak/plugins_reconnect branch September 20, 2019 07:01

benoitf reviewed Sep 20, 2019

View reviewed changes

mmorhun mentioned this pull request Sep 20, 2019

Che Theia build is broken eclipse-che/che#14618

Closed

RomanNikitenko mentioned this pull request Jun 10, 2021

Eclipse Che reloading intermittently eclipse-che/che#19796

Closed

tsmaeder mentioned this pull request Oct 27, 2023

Git plugin issue after network disconnection #12823

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reload plugins on reconnect #6159

reload plugins on reconnect #6159

akosyakov commented Sep 11, 2019 •

edited

Loading

amiramw commented Sep 19, 2019

akosyakov commented Sep 19, 2019

akosyakov commented Sep 19, 2019

svenefftinge left a comment

svenefftinge Sep 19, 2019

akosyakov Sep 20, 2019 •

edited

Loading

svenefftinge commented Sep 19, 2019

akosyakov commented Sep 20, 2019

benoitf Sep 20, 2019

akosyakov Sep 21, 2019

benoitf Sep 20, 2019

akosyakov Sep 21, 2019

benoitf Sep 21, 2019

akosyakov Sep 23, 2019 •

edited

Loading

@@ @@ -80,16 +80,6 @@ export class PluginHostRPC { @@
                       }
                   }
-                  /*

reload plugins on reconnect #6159

reload plugins on reconnect #6159

Conversation

akosyakov commented Sep 11, 2019 • edited Loading

What it does

How to test

Reconnecting tree views

Review checklist

Reminder for reviewers

amiramw commented Sep 19, 2019

akosyakov commented Sep 19, 2019

akosyakov commented Sep 19, 2019

svenefftinge left a comment

Choose a reason for hiding this comment

svenefftinge Sep 19, 2019

Choose a reason for hiding this comment

akosyakov Sep 20, 2019 • edited Loading

Choose a reason for hiding this comment

svenefftinge commented Sep 19, 2019

akosyakov commented Sep 20, 2019

benoitf Sep 20, 2019

Choose a reason for hiding this comment

akosyakov Sep 21, 2019

Choose a reason for hiding this comment

benoitf Sep 20, 2019

Choose a reason for hiding this comment

akosyakov Sep 21, 2019

Choose a reason for hiding this comment

benoitf Sep 21, 2019

Choose a reason for hiding this comment

akosyakov Sep 23, 2019 • edited Loading

Choose a reason for hiding this comment

akosyakov commented Sep 11, 2019 •

edited

Loading

akosyakov Sep 20, 2019 •

edited

Loading

akosyakov Sep 23, 2019 •

edited

Loading