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

Execution dies on Rpc.Invoke #21

Closed
olegklimov opened this issue Dec 26, 2023 · 21 comments
Closed

Execution dies on Rpc.Invoke #21

olegklimov opened this issue Dec 26, 2023 · 21 comments

Comments

@olegklimov
Copy link
Contributor

I added "AAA1" and "AAA2" around the call, only "AAA1" is visible in the logs.

On the screenshot is my attempt to reproduce this again, right after merging #10 .

Windows 11
Debug build from source
refact-lsp downloaded from https://github.com/smallcloudai/refact-lsp/actions/runs/7289748072
dist-x86_64-pc-windows-msvc

IMAGE 2023-12-26 20:49:40

@olegklimov
Copy link
Contributor Author

@digital-phoenix that's a blocker for any other testing at this point :/

Kirill @reymondzzzz might help you, but he doesn't have windows either, only a virtual machine that's not ideal for debugging things like that one.

@digital-phoenix
Copy link
Contributor

digital-phoenix commented Dec 27, 2023

@olegklimov would it be possible to upload the output window data as an external file? Also if you comment out line 150 "info.CreateNoWindow = true;" in RefactLanguageClient.cs do you see a server window pop up at all?

@reymondzzzz
Copy link
Member

Guys, I found commit in lsp binary. This commit broke rpc requests. I will try to fix it ASAP

@reymondzzzz
Copy link
Member

So after 6 hours debug I found reason why it doesn't work. For completion we need text of file and we save it in cache in rust with didOpen/didChanged events. Your code did completion before this events(another ide did event and after call completion). And rust throws exception and rust fall down with panic before and your code restarts rust and call didChange event. After works ok. Oleg found bug with blocked http server. This bug in rust which lsp started in main thread and blocked http server startup. I moved lsp in another thread. It works. So.... My bugfix didn't restart lsp thread and another events doesn't work because thread has panic.

What we need to do:

  • You need rework code with right order of event didChange/didOpen -> completion
  • I need to fix lsp panic with exception. Thread will restart never with any exception.

@digital-phoenix
Copy link
Contributor

@reymondzzzz I believe the latest pull request solves the problem

@reymondzzzz
Copy link
Member

this? #22 You add only stderr-log in args of rust? how it helps?

@digital-phoenix
Copy link
Contributor

@reymondzzzz when I tested it without that addition the rpc call would hang indefinitely and with the code added the rpc call worked correctly.

@digital-phoenix
Copy link
Contributor

@reymondzzzz if you still see have the issue even with the pull request let me know what file type you're testing with.

@reymondzzzz
Copy link
Member

It still doesn't work.

// main.cpp

#include <stdio.h>
#include <stdlib.h>


int ma

I try to complete main

@digital-phoenix
Copy link
Contributor

digital-phoenix commented Dec 27, 2023

@reymondzzzz Can you see if a c# file works better?

@reymondzzzz
Copy link
Member

same

using System;

namespace HelloWorld
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine

@reymondzzzz
Copy link
Member

How --logs-stderr helps? it's only redirect stderr in stdout.

@reymondzzzz
Copy link
Member

@digital-phoenix take this pr smallcloudai/refact-lsp#55 for lsp. It fixes this problem, but log has errors. Fix pls event order bug.
image

@digital-phoenix
Copy link
Contributor

@reymondzzzz I couldn't reproduce the issue you're seeing but I resolved some potentially related issues.

@reymondzzzz
Copy link
Member

Check events. Order is broken. Fix it pls

@olegklimov
Copy link
Contributor Author

🎉 🎉 🎉 we're moving forward 🎉 🎉 🎉

@digital-phoenix
Copy link
Contributor

@reymondzzzz the order of events is correct. This is accomplished in one of two ways. Either the lsp will recognize the extension of a file type and the lsp server will automatically launch. Then the visual studio sdk will handle the server messages automatically. Failing an automatic launch the lsp server will be manually launched and every time the lsp is asked for a completion the code will first check to see if the lsp knows about the file. If it doesn't it will send a didOpen message to the server and create a change handler that will send updates to the lsp whenever the file changes.

Can you make sure to update to your code with the latest pull request and send a file containing all the info from the output window and another file containing all the server output?

@digital-phoenix
Copy link
Contributor

@reymondzzzz let me know if the server still receives getCompletion calls with the latest pull request

@reymondzzzz
Copy link
Member

reymondzzzz commented Dec 29, 2023

I'm trying it.

@reymondzzzz
Copy link
Member

@digital-phoenix with last commits it works correct. Great job!

@olegklimov
Copy link
Contributor Author

OK, it still dies, but less often. It looks like rust doesn't validate inputs very well, and panics again (hypothesis, need to check).

The current problem for this issue is this: the exception between AAA1 and AAA2 doesn't get caught by "catch (Exception)" and doesn't go to the status bar. Maybe there's some kind of a more wide exception, like BaseException in python?

There are two things to fix:

  • panic
  • show error to user

Panic we'll fix elsewhere, but the lack of an error message is what makes this issue much worse -- it looks like it works, but it doesn't.

To reproduce error: start writing new code at the end of any file => lsp will die very soon.

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

No branches or pull requests

3 participants