OmniSharp / omnisharp-vim

Vim omnicompletion (intellisense) and more for C#
http://www.omnisharp.net
MIT License
1.71k stars 169 forks source link

Feedback wanted! stdio #468

Closed nickspoons closed 4 years ago

nickspoons commented 5 years ago

Asynchronous server interactions have been added, with the server switch from the HTTP to stdio version of OmniSharp-roslyn.

In my personal usage the new version has been very good, much faster and smoother. I also hope this will remove some barriers to getting OmniSharp running initially for new users, as the OmniSharp-vim python dependency and OmniSharp-roslyn libuv dependency are both removed.

I'm opening this issue in the hope of getting some feedback from anyone trying out the new server. Bugs should be reported of course, but anyone who doesn't have any problems is most welcome to comment or give a :+1:, and that way I can get a better idea of when to make stdio the default (currently the feature is opt-in only with the g:OmniSharp_server_stdio flag).

jpfeiffer16 commented 5 years ago

I'm trying it out and so far so good. I do notice a lot more speed. I have noticed on a few occasions though that when I pick a code action, it seems like the omnisharp server is duplicating the files in the solution in memory or something because I get a bunch of errors about duplicate/ambiguous classes etc.. Restarting the server fixes it. Still trying to get something that is more reproducible.

stevearc commented 5 years ago

I did a quick check to see if it's a drop-in replacement. I updated to OmniSharp 1.32.19 (64-bit windows) and set let g:OmniSharp_server_stdio = 1. I'm running inside of WSL with neovim and it doesn't appear to be working. Is there an easy way to get a verbose log of the communication to/from the the OmniSharp process?

nickspoons commented 5 years ago

@stevearc yes all requests responses are logged, I meant to add an option to turn that off but forgot. I also forgot the command to open the log, similar to :OmniSharpOpenPythonLog, but the stdio log is next to the python one, in ~/.vim/plugged/omnisharp-vim/stdio.log (if you're using vim-plug)

nickspoons commented 5 years ago

@jpfeiffer16 yes I've experienced that too at times. I think it's a server bug, where server requests sent before the server is fully loaded store the buffer as unattached files. AFAIU the idea is that once the project is loaded in the server, the unattached buffer is recognised as belonging to the project and included, but that doesn't always happen, so in the server you end up with both the unattached buffer and the project file. Hence, duplicates.

Currently I listen for a specific message from the server before I mark the server as "loaded". I'm thinking I need to extend this to listen for "project XYZ" loaded from the server fromall solution projects, which will mean a longer wait before the server accepts requests, but will avoid these errors. Perhaps if the bug is fixed in OmniSharp-roslyn at some stage we can do back to this current behaviours.

Note that this bug also occurs occasionally in the HTTP version.

nickspoons commented 5 years ago

Another thing that might be handy is a statusline function. I have one I use in my config, where a flag indicates the server state - I think I might move this from my config into OmniSharp-vim.

nickspoons commented 5 years ago

@stevearc did you run :OmniSharpInstall after seeing let g:OmniSharp_server_stdio = 1?

nickspoons commented 5 years ago

@stevearc I have added a generic :OmniSharpOpenLog command, which opens the stdio.log in stdio mode and the python.log in HTTP mode. The logging now also respects g:OmniSharp_loglevel (values 'debug', 'info' and 'none') so make sure to let g:OmniSharp_loglevel = 'debug'.

stevearc commented 5 years ago

I didn't run :OmniSharpInstall because I was under the impression that it wouldn't install the Windows version if run from inside WSL.

Found my issue though, I forgot to check the path to the exe and it changed because I unzipped the new server differently. Now it works! I'd recommend printing an error message if the executable path couldn't be found.

Most of the things I use regularly seem to work, but I'm not getting ALE linting. Is there another config change I need to make for that?

nickspoons commented 5 years ago

Oh no you're right, it won't install the correct version inside WSL. In that situation I tend to open a Windows gvim and do the :OmniSharpInstall from there.

Printing some error messages is a good idea, I'll try to set up some listeners.

I'm not sure why you're not getting ALE linting, that works fine for me out of the box I think. I'll try later on with a clean config.

nickspoons commented 5 years ago

@stevearc do you have an up-to-date ALE? The linting depends on its relatively new ALEWantResults autocmd

nickspoons commented 5 years ago

@jpfeiffer16 I have updated the "loaded" listener now so it listens for all projects to be loaded. For me, this appears to have removed the "Duplicate definition" errors.

I have made this optional, so for users who don't experience the duplication issue, or are trying different versions of OmniSharp-roslyn which might not have this problem, you can let g:OmniSharp_server_stdio_quickload = 1

stevearc commented 5 years ago

Found the issue. I'm using .vim/ftplugin/cs.vim to set my OmniSharp config, including let g:OmniSharp_server_stdio = 1. Unfortunately, the ALE autocommand is set inside plugin/OmniSharp.vim, which is executed when vim loads (well before filetype events). I got it working by moving my config into .vimrc.

I think an easy fix for this would be to always register the s:ALEWantResults function with the autocommand and do the variable check & early return there.

nickspoons commented 5 years ago

@stevearc good idea, done.

stevearc commented 5 years ago

Everything's working for me now! I'll keep using it and see if anything else turns up.

I've noticed it starts up much faster. I used to have to wait 1-2 minutes for the server to spin up, but now it's closer to 10 seconds. Awesome! Thanks for all your work on the upgrade!

nickspoons commented 5 years ago

You're welcome! Yeah it's pretty speedy, I'm finding it much nicer to work with - and more reliable too, since updating the listeners for completed projects. Being able to listen for all server events as they arrive rather than polling is really nice.

There are a few more improvements I can build on on top now, like listening for unrequested diagnostics, and the TextChanges mentioned (4 years ago!) in #215. Currently we don't receive changes that way from the server but that could well be the way we request them ... I'll take a look when I find some more time.

stevearc commented 5 years ago

Back again 😅

NeoVim apparently doesn't have deletebufline. Some more information here: https://github.com/w0rp/ale/issues/1739

I'm also seeing that when I :w a file the buffer "bounces" from written back to dirty. It seems to be a problem with my autoformatter. I have it configured to call OmniSharpCodeFormat on BufWritePre. Is it possible that this operation has become asynchronous with the stdio implementation? Is there any way to force it to complete synchronously?

nickspoons commented 5 years ago

Oh I didn't realise deletebufline was new, and not in neovim. I'd better do something about that - I'll add a patch check or something.

Yes :OmniSharpCodeFormat is asynchronous now, so it's not going to work in a BufWritePre. We could do 2 things:

  1. Add an option to make it synchronous, which involves sleeps until the response has returned (this is how OmniSharp#GetCompletions works when being run as omnifunc)
  2. Add an optional callback, so you'd be able to do something like this:
    " ~/.vim/after/ftplugin/cs.vim
    autocmd BufWriteCmd <buffer> call OmniSharp#CodeFormat({->execute('noau w')})

    I haven't tried this, but the docs say that BufWriteCmd is expected to do its own writing, so using this method should work.

I prefer option 2 - what do you think?

nickspoons commented 5 years ago

@stevearc I have implemented option 2. I tested it out and it works well I think - just make sure you use :noautocmd in the callback, or you'll start an endless loop!

function! s:CBCodeFormat() abort
  noautocmd write
  set nomodified
endfunction
autocmd BufWriteCmd <buffer> call OmniSharp#CodeFormat(function('s:CBCodeFormat'))

I also added a test for deletebufline.

stevearc commented 5 years ago

Works! Thanks for the fast fix! I'll keep an eye out for other issues 🙃

psuong commented 5 years ago

Hey there, the stdio implementation works quite well on my side too! Even on Unity side it's quite responsive :)

nickspoons commented 5 years ago

Thanks @psuong, that's good to hear. It seems to be working very well, I'd like to make it the default now but I need to work out how to do it smoothly, ideally it should detect existing HTTP servers and use the correct mode for those. Perhaps echo a recommendation to switch to stdio (for vim8 and neovim which can handle it).

jmzagorski commented 5 years ago

@nickspoons thank you for all the hard work with this repo by keeping it up to date and being responsive to issues. I switched to stdio in WSL and GVIM and it is super fast! I had no issues following your instructions. Great work!

Incubatio commented 5 years ago

Had a working version before, tried to upgrade everything and use stdio, can't get it to work anymore. The weirdest part is that I don't have any particular error from :OmniSharpOpenLog

Log report no error and stops after it started up, last log being: process 43572 run {"Event":"log","Body":{"LogLevel":"INFORMATION","Name":"OmniSharp.Stdio.Host","Message":"Omnisharp server running using Stdio at location '/Users/myuser/dev/csharp/myApp' on host -1."},"Seq":14,"Type":"event"}

Using shortcut gs do display :OmniSharpGotoDefinition in vim, but nothing happens.

Here is my environment:

Mac os 10.13.5
vim 8.1.1450 and MacVim 8.1.1950
OmniSharp v1.32.20
let g:OmniSharp_server_stdio = 1
let g:OmniSharp_server_use_mono = 0
All vim plugin including ale are to latest version (installed via vundle)
nickspoons commented 5 years ago

@Incubatio do you mean you have previously had a working Http version, or a working Stdio?

It looks like you have installed the Stdio with :OmniSharpInstall. And I assume you have restarted Vim after that?

Could you try clearing the log, adding let g:OmniSharp_loglevel = 'debug' to your .vimrc, and then loading vim with a .cs file and try some things please, i.e. :OmniSharpGotoDefinition, :OmniSharpHighlightTypes, :OmniSharpGlobalCodeCheck? Then post the full log here, or link to a pastebin or something.

scottbass47 commented 5 years ago

I'm having a very similar problem. I'm using the new Stdio server with Neovim but every OmniSharp command I results in nothing happening. Here is a pastebin with let g:OmniSharp_loglevel = 'debug' set and running a few of the commands you listed. https://pastebin.com/VVLH4Tb6

Incubatio commented 5 years ago

Yes it was previously working with the http version. It took me few hours to get it to work the first time (as there was a http connection error with very recent roslyn server version), and I spent 2 more hour yesterday trying to update it to stdio.

I've installed it and re-installed it with different options many times, but yes the main way of doing it was via :OmniSharpInstall.

let g:OmniSharp_loglevel = 'debug' is already in my vimrc as you mentioned it in this thread.

As I have literally no log on my work machine, I'm gonna try on on a different machine where I never installed it. I will post again later today.

Incubatio commented 5 years ago

Re-installed everything on another machine at home, same result.

what I do: After everything is installed, I open a cs file (either from a simple dotnet console application, or from a more complex unity project), then I tried twice :OmniSharpGotoDefinition, :OmniSharpHighlightTypes, :OmniSharpGlobalCodeCheck

log generated via :OmniSharpOpenLog: https://pastebin.com/yxc5ykTu

As you can see still no log about Omnisharp commands, they fail, and fail silently.

It is weird because changing the loglevel doesn't log anything more. I went directly in the source to force set "debug", doesn't log anything more. Went back again and changed the log function to:

  function! s:Log(message, loglevel) abort
    call writefile([a:message], s:logfile, 'a')
  endfunction

aaaannd still doesn't log anything more.

I'm gonna go through your sources, read it and try to add some logging ...

Incubatio commented 5 years ago

Ok I seem to have identified the source of the problem: in stdio.vim:124, when building the raw request, job.loaded is always false , which leads to always early return. When I remove the condition || !job.loaded, omnisharp seem to work properly again.

now i need to figure why job.loaded is always false.

Incubatio commented 5 years ago

stdio:27, in ListenToServer you're expecting a communication that never happen in my environments "OmniSharp.MSBuild.ProjectManager". And there are no timeout or no contraption to notify the user that he didn't received that message and that your app didn't load.

I'm not familiar at all with the whole omnisharp process, so I will leave it to someone else to apply a fix that could work for everyone. In the meantime a quickfix is to enable let g:OmniSharp_server_stdio_quickload = 1

Note: I would suggest to rename ListenToServer to handleServerRequest. ListenToServer feels like it will be executed only once, it infers that your code is switching to a state where you're listening, like you're opening a socket to listen to the server.

nickspoons commented 5 years ago

Oh great debugging, thanks! That's very interesting that you aren't getting any of those events. Yes adding a timeout there is a good idea, I'll do that.

I'm currently trying to improve that area as I don't think it works properly as it is anyway, see OmniSharp/OmniSharp-roslyn/issues/1521

nickspoons commented 5 years ago

@scottbass47 yours looks a bit different, as you at least are getting your requests logged. You're using neovim on Windows by the looks - is that using nvim-qt? Or a terminal (which one?)?

~Although either way, the same project loading lines from the server are missing, so it looks similar to what @Incubatio describes.~ I misread the logs on mobile, the project loaded lines are included in your output.

nickspoons commented 5 years ago

I think it might be helpful if I add some feedback messages. "Starting server for xxx.sln", "xxx.sln server loaded", "Failed to receive server loaded notification for xxx.sln after 30 seconds." etc. After the load timeout period (default to 30 seconds) it can echo the above warning, but also mark the server as loaded and hope for the best.

This will all be quite interesting when loading multiple solutions, but I'll include an option to suppress these messages too.

In my personal configuration I have a statusline flag which shows the server load state which I find useful. I tried adding a generic statusline function to OmniSharp-vim but it was messy, and didn't need to be part of the plugin itself. Instead, I'm thinking of creating an opinionated OmniSharp-vim companion plugin (working title: vim-sharpenup) which sets default mappings, includes the statusline function and instructions/options for including it in a custom 'statusline' as well as airline/lightline, and also experimental features like the "code actions available" function described in the wiki.

scottbass47 commented 5 years ago

Yes @nickspoons I'm using nvim-qt on Windows

nickspoons commented 5 years ago

@Incubatio I've added a timeout as described above, in #480. This defaults to 30 seconds, after which the server is marked as "loaded". A message is also echoed to say that either the server has loaded normally, or it has loaded after the timeout (these messages can also be switched off with a variable). Hopefully this helps.

I'll keep looking into this because it isn't an entirely satisfactory solution. At this stage I'm wondering if the version of MSBuild that OmniSharp-roslyn is using might have something to do with which notifications are emitted from the server?

nickspoons commented 5 years ago

@scottbass47 I can reproduce your issue in nvim-qt in Windows. The server appears to be loading correctly but never receives any responses to requests. I'll try to look into it further tonight.

Edit: The same thing happens in terminal neovim on Windows.

Incubatio commented 5 years ago

@nickspoons, thx for the patch.

You are addressing the problem, however for 30s omnisharp commands will fail silently (and 30s is terribly long in vim :P ).

I think it could be useful to notify the user when he tries to use a command and the server is not loaded. I'm thinking displaying an error in vim and the logs via s:Request or s:RawRequest :

if !job.loaded
 let msg = 'Omnisharp is not loaded yet. For quickload, enable OmniSharp_server_stdio_quickload';
 call s:Log(msg, 'info');
 echomsg printf(msg)
endif

As a rule of the thumb, I think there should be alway log when you return early

Having it silent and seeing on this thread that people could have it work is what tricked me into installing it and re-installing it again. If I had been displayed error messages, I would have had a start point in the code and be able to debug very quickly from there.

Incubatio commented 5 years ago

The For quickload, enable OmniSharp_server_stdio_quickload being there temporarily until omnisharp-vim can start properly on each platform.

nickspoons commented 5 years ago

@Incubatio As I described, the 30s timeout is configurable, let g:OmniSharp_server_loading_timeout = 5 for a shorter timeout (or let g:OmniSharp_server_stdio_quickload = 1 as you mentioned).

The problem with echoing warnings when the server is not yet loaded is that the server will typically receive several requests before it is loaded due to normal edits/buffer switches (e.g. highlighting, ALE code checking, updating the buffer etc.). All of these should not be echoed to the user ... but logging them in the stdio.log file would be a good idea I agree.

Another issue is that I am trying to implement "Replay requests on load" functionality for some endpoints (ALE code checks and highlighting) so that these will automatically be replayed once the server is available: https://github.com/nickspoons/omnisharp-vim/tree/feature-replay-requests-on-load. This works fine ... except that because we can't reliably detect the server being loaded, they often replay too early. So again, I don't want to echo warnings about these.

Incubatio commented 5 years ago

Usually if any piece of software ignores interactions or commands for few seconds, one might assume it's bugged and either restart it or discard it.

My motivation is the spreading and the success of this plugin, so I really would like a good first user experience.

I'm sure there is a better solution than my first proposition. Here are few on the top of my head:

Anyway, sorry for insisting. If you find an elegant solution / process to solve the problem I'd be happy to help.

nickspoons commented 5 years ago

@Incubatio all input gratefully received.

Usually if any piece of software ignores interactions or commands for few seconds, one might assume it's bugged and either restart it or discard it.

That's fair I suppose although with language servers some delay seems to be considered "reasonable" these days. None of the LSP clients I've tried give more feedback while loading their servers.

I also think that we'll be able to improve things greatly with the replay-events once they work well, as ideally all commands will be able to be replayed as soon as the server is ready, so early requests won't fail, they'll just be slow initially (as they also are in VSCode and VS). Statusline functions etc. will hopefully also help. For me, the server usually loads within 5s (small project) and 10-15s (large solution like OmniSharp-roslyn).

At this stage I still consider stdio to be experimental since it is opt-in, and I hope to have the experience much smoother before making it the default. As you say, the best solution is to have proper detection of the server loading, and that's what I'm aiming for. Really hoping for some feedback from the OmniSharp-roslyn guys about that, but I'll keep tinkering with it.

I've spotted {"Event":"started","Body":null,"Seq":16,"Type":"event"} in my logs and scottbass47's, couldn't we use that ?

I have tried it, but it is actually fired before the projects are loaded, which means it is way too early. In scottbass47's log you'll see that the "started" event comes before the "Successfully loaded..." event.

Printing a message to the user could happen solely on user interaction, avoiding replayed or system request, or any other source of your choice (that's just an additional Boolean).

Good idea.

Displaying the server status in vim, ex: "Omnisharp: initializing ...", until it's initialized (when first request goes through).

Yeah that's what a statusline function will provide, when I can get around to making one. Or did you have something else in mind?

The obvious other solution is to poll the server with one endpoint or another until a sensible response is received. However this also doesn't work currently with at least some endpoints, as it can lead to a buffer somehow being loaded twice in OmniSharp-roslyn's memory, so all classes/methods are marked as duplicates (see my description in the OmniSharp-roslyn issue I linked above). However it might be that a different endpoint could be used without this issue, e.g. the /highlight endpoint which doesn't seem to trigger this condition. I suppose I'm hesitant to go the polling way because it's so clumsy, when we've got these events coming from the server. But since the events don't seem to be giving enough information we may need to do this.

I have started debugging OmniSharp-roslyn to try to understand the event system better, but haven't found enough time to really get into it yet. I also tried looking at the OmniSharp-vscode code to see if they had a sensible way of dealing with this but I can't really work it out :(

nosami commented 5 years ago

I just tried this on a new machine using the .vimrc from the README and it works great! Nice job.

My only issue was that I copied the README example config verbatim but didn't run :PlugInstall (it's been years since I've used Vim now, forgive me, I've reverted to being a noob!) so maybe you need a comment in the example config to mention that. Took me a little while to figure out what I needed to do to get OmniSharp installed.

Once it was loaded though, it offered to installed the server for me which worked flawlessly. The new syntax highlighting is fantastic!

nickspoons commented 5 years ago

Thanks very much @nosami, that means a lot from you! Yes the server-installation really lowers the entry barrier, that was some great work by @axvr.

Thanks for the feedback about the README. I've updated it.

And I'll take this opportunity to thank you very much for OmniSharp, in all its forms! It all came about before I was a Vim user at all but I've come to realise that it was ahead of its time, and I'm loving seeing all the continued work going on with it (particularly the OmniSharp-roslyn refactorings etc. coming in lately).

jpfeiffer16 commented 5 years ago

@nickspoons I am getting nvim-qt setup on windows as well and I am also seeing @scottbass47 's issue. Have you by chance gotten anywhere on that? I wrote a stdin-stdou proxy layer for this to try and troubleshoot it and it ended up actually fixing this issue oddly enough. Happy to take this somewhere else so as not to clutter up this thread.

nickspoons commented 5 years ago

@jpfeiffer16 no I didn't get any further with the nvim-on-windows bug. Can you share your code? Either in a PR or just a link to a branch of your fork or something?

jacquesh commented 5 years ago

I'm sorry if this is off-topic or not a fault with omnisharp-vim, but I started experiencing it when I upgraded all my packages recently so I thought it might be related.

Does anybody have an example vimrc that gives working autocomplete in either vim or neovim? Since updating I've been experiencing significant pauses while typing. I use neocomplete but I've also tried to get it working in neovim using ncm2 (I would prefer to move to nvim but I'm waiting to get a omnisharp & autocomplete functioning there) and its much worse there. In both cases the issue goes away if I disable the autocomplete plugin. I'm using Vim 8.1.517 from Windows 10 (no WSL) but I originally experienced the issue on Vim 8.0.311. Both :OmniSharpHighlightTypes and :OmniSharpGlobalCodeCheck function correctly.

I tried using only the vimrc in the README file but for some reason it always thinks that OmniSharp still needs to be installed (its already installed in the default location). If I delete my installation and accept the prompt to install, it complains that Installation to "<User directory>\.omnisharp\omnisharp-roslyn" failed inside PowerShell. If I run the installation script manually from powershell it succeeds, but then Vim still complains that OmniSharp needs to be installed.

I can reliably reproduce the issue by creating a new project/file, going to the class name, and hitting i:<space> and then continuing to type. Vim pauses visibly when I hit space. I imagine it is trying to populate the autocomplete list synchronously but I have no idea why (or indeed how to even determine where the delay might be, I don't see any timestamps in the log so can't check that). As a comparison I tried using ncm2-racer with ncm2 to auto-complete for rust and that worked very smoothly (although admittedly that is using an autocomplete implementation dedicated to ncm2 so maybe thats not fair).

nickspoons commented 5 years ago

@jacquesh do you have let g:OmniSharp_server_stdio = 1 in your .vimrc? If not, this is certainly off topic.

OmniSharp-vim doesn't have any sources/integration with ncm2 - I've never tried it, I have no idea if they're easy to add? If so, you're welcome to give it a go and submit a PR. But yes it is most likely using standard vim omnicompletion, which is synchronous.

OmniSharp-vim (with g:OmniSharp_server_stdio) does have an asyncomplete source, and for me autocompletion with asyncomplete works great.

Please open another issue for your installation problems, it sounds like something is going wrong there.

nickspoons commented 5 years ago

@jacquesh actually, could you try the suggestions in this comment to see if they solve your installation issue?

jpfeiffer16 commented 5 years ago

@nickspoons : Here is the little program I wrote https://github.com/jpfeiffer16/omnisharp-vim/blob/proxy/stdioproxy/Program.cs . I publish it with dotnet publish -r win-x64 --self-contained and then point g:OmniSharp_server_path to the exe on windows. The code might need a little tweaking if I missed anything for platform independence.

nickspoons commented 5 years ago

Oh that's interesting, thanks @jpfeiffer. Did you specifically write this to help debug neovim/stdio?

jpfeiffer16 commented 5 years ago

Yep. I wrote it to debug the issue with neovim not firing responses to requests. The fact that it fixes it makes me start to think it might be the way neovim expects data to be chunked to stdout since Im proxing stuff one byte at a time and that seems to clear the problem up. I may be way over analysing it though.

On Sat, Jun 15, 2019, 2:28 AM Nick Jensen notifications@github.com wrote:

Oh that's interesting, thanks @jpfeiffer https://github.com/jpfeiffer. Did you specifically write this to help debug neovim/stdio?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/OmniSharp/omnisharp-vim/issues/468?email_source=notifications&email_token=ACCVOZDRMA6MGUCVY3AMCP3P2SR3JA5CNFSM4HOBC6Z2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXYTHKI#issuecomment-502346665, or mute the thread https://github.com/notifications/unsubscribe-auth/ACCVOZHVZJ3KWXLAZULD75LP2SR3JANCNFSM4HOBC6ZQ .