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

Reconnect same host plugin process and client #5257

Merged
merged 1 commit into from
May 30, 2019

Conversation

amiramw
Copy link
Member

@amiramw amiramw commented May 25, 2019

Fixes problem of plugins not working after short disconnection.
Keep disconnected host plugin process alive for at least one minute and
reconnect it if the client is the same.

Signed-off-by: Amiram Wingarten [email protected]

@amiramw amiramw force-pushed the relink-plugin-host branch from 3d2c685 to 9e0cab0 Compare May 27, 2019 13:39
@amiramw amiramw force-pushed the relink-plugin-host branch from 9e0cab0 to a907647 Compare May 28, 2019 21:41
@akosyakov
Copy link
Member

akosyakov commented May 29, 2019

We could accept it as a temporary fix. I don't like it much since it will cause other issue, i.e. you close a window but process tree is hanging for one minute occupying different ports and so on. So if someone open a new window within one minute and tries to launch something it is not going to work. Maybe timeout can be smaller like 5-10s. @amiramw for how long you have connection issues?

I also believe we already have general 30s timeout over websocket connection. Not sure that we need a specific one for plugin system. I would prefer that if websocket connection is gone then everything gone as well and is not kept for another minute.

@amiramw amiramw force-pushed the relink-plugin-host branch 3 times, most recently from 4d17c52 to fac6886 Compare May 29, 2019 23:05
@amiramw
Copy link
Member Author

amiramw commented May 29, 2019

@akosyakov We are facing disconnections that take few seconds. I reduced timeout to 10 seconds and added an ENV variable so that we can override it if needed. If you want I can make it even lower. Also I fixed the other code review comments.
Even as temporary solution for 0.7.0 this would be a major improvement in theia productivity for us.
I think that for few seconds of disconnection it is good not to be forced to execute all plugins and vscode extensions again.
For later version I can take a look into a full solution that will work also after the timeout and also after theia restart.

@akosyakov
Copy link
Member

I think that for few seconds of disconnection it is good not to be forced to execute all plugins and vscode extensions again.

It is error prone. If a ws connection is gone then some data can be lost which should have been sent, like text edits, so state of plugin on backend is different from frontend state leading to all kind of functionality bugs. If you want to really reuse backend plugins you should make sure that they sync state with frontend when a client reconnects. It is not an easy task as well.

Usually we preserve ws connection for 30s on network issues that data are delivered eventually. In this case it's possible to reuse plugins. If a ws connection is not preserved the easiest strategy is to kill a plugin and start a new one to make sure that it is in sync with frontend state.

On your network, apparently, somehow ws connection is got killed on frontend or backend too early by network errors.

Fixes problem of plugins not working after short disconnection.
Keeps disconnected host plugin process for at least one minute and
reconnect it if the client is the same.

Signed-off-by: Amiram Wingarten <[email protected]>
@amiramw amiramw force-pushed the relink-plugin-host branch from fac6886 to 79ef4a2 Compare May 30, 2019 07:27
@amiramw
Copy link
Member Author

amiramw commented May 30, 2019

@akosyakov I see your point. Maybe timeout small enough can make these inconsistencies pretty rare.
We have an issue with the way nginx ingress controller handles route changes which cause short disconnection.
It can be reproduced locally with proxy server copied from here: #4590. Terminate host plugin process is triggered immediately when killing the proxy.

Anyway as a temporary solution for 0.7.0 I would appreciate if this could be merged. I'll try to investigate in parallel the second approach.

@amiramw
Copy link
Member Author

amiramw commented May 30, 2019

@akosyakov @benoitf If this is acceptable as temporary solution I would be happy if you could merge it before 0.7.0 release today 🙏

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

ok, let's try ... i just afraid that we open up another can of issues here instead of solving the real issue

@akosyakov akosyakov merged commit 797db75 into eclipse-theia:master May 30, 2019
@amiramw amiramw deleted the relink-plugin-host branch May 31, 2019 09:42
@akosyakov akosyakov mentioned this pull request Sep 11, 2019
60 tasks
akosyakov added a commit that referenced this pull request Sep 19, 2019
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 <[email protected]>
akosyakov added a commit that referenced this pull request Sep 19, 2019
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 <[email protected]>
akosyakov added a commit that referenced this pull request Sep 19, 2019
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 <[email protected]>
akosyakov added a commit that referenced this pull request Sep 20, 2019
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 <[email protected]>
akosyakov added a commit that referenced this pull request Sep 20, 2019
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 <[email protected]>
akosyakov added a commit that referenced this pull request Sep 20, 2019
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 <[email protected]>
akosyakov added a commit that referenced this pull request Sep 22, 2019
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 <[email protected]>
akosyakov added a commit to akosyakov/theia that referenced this pull request Feb 24, 2020
Also:
- fix eclipse-theia#5829: ensure that each plugin is loaded only once
- fix eclipse-theia#6186: start only one plugin host process for the same host instead of a new for each load as before
- revert eclipse-theia#5257: Reconnect same host plugin process and client" 797db75

Signed-off-by: Anton Kosiakov <[email protected]>
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

Successfully merging this pull request may close these issues.

3 participants