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

[issue-114] Infoview should indicate when server is not started #159

Merged
merged 51 commits into from
Jun 1, 2022

Conversation

marianaalanis93
Copy link
Collaborator

@marianaalanis93 marianaalanis93 commented Mar 25, 2022

This PR contains the next adds related with issue #114 :

  • Fix error scenarios (error handling) when server crashes.
  • Add an appropriate message when the server dies and doesn't have the ability to recover itself.

Note: This PR needs to merge #162 before merging it.

@Vtec234
Copy link
Member

Vtec234 commented Apr 1, 2022

Hi! Thank you for looking into this! This is still a draft so I am a bit early to comment, but I just wanted to check-in to see where the changes are headed. I was surprised to see the changes in info.tsx, as my initial guess for how to fix #114 would be to only change the top-level component in main.tsx. Specifically, we already have logic there to handle a server which isn't yet running, so it seems this change would naturally fit there as well. What is the motivation for changing components deeper in the UI hierarchy?

@Vtec234
Copy link
Member

Vtec234 commented Apr 1, 2022

(If the reason is to support multi-server workspaces, I think something like this makes sense and if it works, great. We will however need more changes to really support that. For example there is only one serverInitializeResult at the moment.)

@marianaalanis93
Copy link
Collaborator Author

Hi! Thank you for looking into this! This is still a draft so I am a bit early to comment, but I just wanted to check-in to see where the changes are headed. I was surprised to see the changes in info.tsx, as my initial guess for how to fix #114 would be to only change the top-level component in main.tsx. Specifically, we already have logic there to handle a server which isn't yet running, so it seems this change would naturally fit there as well. What is the motivation for changing components deeper in the UI hierarchy?

Thanks @Vtec234 for your comments! (the more the better so no worries). Yeah, at the beginning I just wanted to add the message inside main.tsx but even though the message is displayed correctly ("server unavailable...") the section containing "All messages" remains static (and maybe it's a bit confusing for the user to keep seeing this section if the server is dead), that's why I was checking to change info.tsx logic, until now I'm blocked because I need to catch the exception related to microsoft/vscode-languageserver-node#825. Still investigating another alternatives :)

@marianaalanis93
Copy link
Collaborator Author

Hi! Thank you for looking into this! This is still a draft so I am a bit early to comment, but I just wanted to check-in to see where the changes are headed. I was surprised to see the changes in info.tsx, as my initial guess for how to fix #114 would be to only change the top-level component in main.tsx. Specifically, we already have logic there to handle a server which isn't yet running, so it seems this change would naturally fit there as well. What is the motivation for changing components deeper in the UI hierarchy?

Also, if you terminate the server manually the main.tsx is not updated anymore, so maybe we need to add more logic for re-render this part

@lovettchris
Copy link
Contributor

Update: Mariana and I figured out a solution by adding a new serverStopped event on the InfoViewApi.

@marianaalanis93 marianaalanis93 marked this pull request as ready for review April 5, 2022 17:25
lean4-infoview/src/infoview/messages.tsx Outdated Show resolved Hide resolved
lean4-infoview/src/infoview/main.tsx Outdated Show resolved Hide resolved
vscode-lean4/package-lock.json Outdated Show resolved Hide resolved
vscode-lean4/package-lock.json Outdated Show resolved Hide resolved
vscode-lean4/package-lock.json Outdated Show resolved Hide resolved
vscode-lean4/src/leanclient.ts Outdated Show resolved Hide resolved
vscode-lean4/src/leanclient.ts Outdated Show resolved Hide resolved
vscode-lean4/src/leanclient.ts Outdated Show resolved Hide resolved
vscode-lean4/src/leanclient.ts Outdated Show resolved Hide resolved
vscode-lean4/src/leanclient.ts Outdated Show resolved Hide resolved
@Vtec234
Copy link
Member

Vtec234 commented May 13, 2022

But from what I can tell there's no way to detect such a crash and the watchdog even ignores all requests afterwards, or did I miss something?
This is an interesting one - I notice the "worker" process dies but the "--server" process does not.

Yes, the watchdog process is meant to never crash unless we have a bug in Lean core, whereas worker processes may crash given inputs such as the unsafeCast. The current logic in the watchdog attempts to transparently restart the worker and re-send all the in-flight requests that it can to the new worker. I'm not certain, but IIRC we don't send any kind of "worker crashed" message to the client, although some of the in-flight requests do return to the client with the "Server process for .." error that Chris points out.

@Vtec234
Copy link
Member

Vtec234 commented May 13, 2022

Having said all that, I took this PR to be fixing what happens when the watchdog (i.e. lean --server) is not started, whereas the restarting behaviour of file workers seems like a different issue altogether. So the issue of a worker that crashed on e.g. unsafeCast and then restarts transparently seems orthogonal.

@lovettchris
Copy link
Contributor

Having said all that, I took this PR to be fixing what happens when the watchdog (i.e. lean --server) is not started, whereas the restarting behaviour of file workers seems like a different issue altogether. So the issue of a worker that crashed on e.g. unsafeCast and then restarts transparently seems orthogonal.

I agree, I filed this as a new issue #175, just so we can get this one finished sooner than later.

@lovettchris lovettchris requested a review from gebner May 21, 2022 01:50
@marianaalanis93
Copy link
Collaborator Author

@gebner solved this discussion: #159 (comment)

@lovettchris
Copy link
Contributor

@gebner, this should be good to go.

@gebner
Copy link
Member

gebner commented Jun 1, 2022

Thanks!

@gebner gebner merged commit dc33aae into master Jun 1, 2022
@gebner gebner deleted the issue-114 branch July 20, 2022 07:42
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.

4 participants