Probing language server might not be needed

[DoctorKrolic]

I've done a simple experiment: added an immediate throw to the language server and got this:

With probing:

Without probing:

I think, both outputs makes it clear of what exactly happened, so no benifits of having probe phaze for that. At the same time in theory probe makes startup time somewhere 2 times slower since probing server involves starting its process (which on its own is not a trivial operation), jitting server code, initializing a bunch of static state and all of that is just to throw this work away a second later. So I think, we should remove probe phaze entirely to improve startup time and simplify existing code both on server and client side

[tintoy]

Hmm, I vaguely remember it was there because of weird runtime-mismatch edge cases etc. Will look over the commit history this weekend 🙂

[DoctorKrolic]

If runtime mismatches the language server client is not able to start the server the same way as we. The point is that error handling should have improved with the latest versions of the library (remember the PR where I upgraded it), so even if something goes wrong when starting the server, we get good logs about the failure regardless of the fact, whether we do probing or not. Thus IMO probing no longer has advantages it was introduced for

[DoctorKrolic]

@tintoy Have you found why probing was introduced? Are we safe to remove it?

[tintoy]

Sorry I haven’t had time to look at this yet, I’m hoping to check it out this morning :-/

[tintoy]

Related:

https://github.com/tintoy/msbuild-project-tools-vscode/commit/4fe68abcb3893a8d48094738e767f5917789f93f https://github.com/tintoy/msbuild-project-tools-server/commit/f749fa29d0e40384d8a6f25df3c0eff212f8ed69

[tintoy]

Will have a proper look later this morning but I think from looking at the commits, probing has historically proved necessary because ordinarily the language server’s STDIN/STDOUT are used for LSP communications and the LSP client did not capture STDERR (and if we can’t find a compatible runtime then our code may not be run at all; in this scenario, we need to be able to capture all output from running dotnet <language-server>.dll).

[tintoy]

Sorry haven’t forgotten about this, just been too busy with work! Hoping to check it out today.

[tintoy]

Having had another look at this, I think probing is still needed (e.g. STDOUT vs STDERR issue), but we could switch to probing only if the language server does not launch correctly. How does that sound?

[DoctorKrolic]

e.g. STDOUT vs STDERR issue

If this is the only issue then we can still drop probing in case we manage to use isolated runtime again (https://github.com/tintoy/msbuild-project-tools-vscode/issues/89). I have an idea of how we can do that, but I need some time and willpower to sit down and check whether my solution is correct. Unfortunatelly, I can only do that on the next weekend since I am very busy at the moment

[tintoy]

No problem; I’ve been super busy too, it’s that time of year I think 🙂

[DoctorKrolic]

Considering that we are back to using isolated runtime now do you think we can safely drop server probing? The runtime mismatch error is no longer possible and I don't think we can get stderr output in any other way since we catch all exceptions in server's Main. And even if we can I don't think a possibility of an edge-case error is worth ~1.5x startup time increase anyway

[tintoy]

Yep, I think we should be ok. I’ll still see what I can do about automating some basic integration testing.