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

Omnisharp is stuck at starting phase with nothing happening #804

Closed
narf opened this issue Oct 8, 2016 · 9 comments
Closed

Omnisharp is stuck at starting phase with nothing happening #804

narf opened this issue Oct 8, 2016 · 9 comments

Comments

@narf
Copy link

narf commented Oct 8, 2016

This is probable more of an omnisharp error, but I post the problem here you guys probably know better what to do with this.

Environment data

dotnet --info output:

.NET Command Line Tools (1.0.0-preview2-003131)

Product Information:
Version: 1.0.0-preview2-003131
Commit SHA-1 hash: 635cf40e58

Runtime Environment:
OS Name: ubuntu
OS Version: 16.04
OS Platform: Linux
RID: ubuntu.16.04-x64

VS Code version: 1.5.3
C# Extension version: 1.4.1

Steps to reproduce

Try to install on linux with mono flavour and a mono version <4.0.1 (or maybe no mono installed)

Expected behavior

An error should be displayed

Cannot start Omnisharp because Mono version >=4.0.1 is required.

Actual behavior

No error is displayed, the omnisharp server just hang on startup stage

Starting OmniSharp server at 10/8/2016, 7:02:23 PM
Target: /blah/blah/blah/blah.sln

How to fix

in out/omnisharp/launcher.js change

// Original code
function launchNixMono(details) {
    return new Promise(function (resolve, reject) {
        return canLaunchMono().then(function () {
            var args = details.args.slice(0); // create copy of details.args
            args.unshift(details.serverPath);
            var process = child_process_1.spawn('mono', args, {
                detached: false,
                cwd: details.cwd
            });
            return resolve({
                process: process,
                command: details.serverPath,
                usingMono: true
            });
        });
    });
}

to

// Fixed code
function launchNixMono(details) {
    return new Promise(function (resolve, reject) {
        canLaunchMono().then(function () {
            var args = details.args.slice(0); // create copy of details.args
            args.unshift(details.serverPath);
            var process = child_process_1.spawn('mono', args, {
                detached: false,
                cwd: details.cwd
            });
            resolve({
                process: process,
                command: details.serverPath,
                usingMono: true
            });
        }, reject);
    });
}

Why are they returning a promise in a promise? I think this is not how es6 Promise works, the callback of the new Promise thing should not return anything. The whole code return things inside its callback it would probably be better to rewrite them.

@narf narf changed the title Omnisharp is stuck at staring phase with nothing happening Omnisharp is stuck at starting phase with nothing happening Oct 8, 2016
@matthamil
Copy link

matthamil commented Oct 9, 2016

The original code you point out should be fine. In your example,

function launchNixMono(details) {
    return new Promise(function (resolve, reject) {
        canLaunchMono().then(function () {
            var args = details.args.slice(0); // create copy of details.args
            args.unshift(details.serverPath);
            var process = child_process_1.spawn('mono', args, {
                detached: false,
                cwd: details.cwd
            });
            resolve({
                process: process,
                command: details.serverPath,
                usingMono: true
            });
        }, reject);
    });
}

the call to canLaunchMono() would fire, and the code dependent upon launchNixMono may or may not happen after canLaunchMono() completes. By returning canLaunchMono(), this guarantees that the code will wait for canLaunchMono() to complete (or fail) before continuing. This also allows you to call .then() and .catch() on launchNixMono elsewhere and expect that the canLaunchMono has resolved (or failed).

TL;DR: by removing the return statement, you are no longer ensuring that all of the async code waits for other async code to complete. The code should be fine as is. The problem probably lies elsewhere ¯_(ツ)_/¯

@narf
Copy link
Author

narf commented Oct 10, 2016

I may be missing something there but I think that you are wrong, returning the call to canLaunchMono inside the Promise executor has no effect there since we already returned new Promise(...) from launchNixMono.
This mean that async termination of the launchNixMono task is handled by resolve/reject callbacks.

You can see thereafter in the code you quoted that every path is calling either resolve or reject and thus the async termination is fully handled which was not the case in the original code.

And I repeat, returning inside a Promise executor (the callback inside the new Promise(executor)) has no effect (I tested it and the doc on ES6 Promise does not specify anything about the return type or usefulness to return something of the executor callback).

@matthamil
Copy link

Ah, I think you are right @narf . My apologies. Thanks for the opportunity to learn :)

@DustinCampbell
Copy link
Member

yeah, this is totally a bug. I don't know what I was thinking when I wrote that. 😄

@DustinCampbell
Copy link
Member

Sometimes I'll return a resolve or reject to simplify control flow in a more complex promise. This is totally unnecessary here though.

@DustinCampbell
Copy link
Member

FWIW, it's not clear to me that this would fix your issue based on the information that you provided. The C# extension does not use Mono by default on ubuntu 16.04. Instead, it will use the Ubuntu 16.04 flavor of OmniSharp, which is a standalone .NET Core app. @narf, did you verify that this actually fixes your problem?

@narf
Copy link
Author

narf commented Oct 11, 2016

Adding the reject to the canLaunchMono().then fixed the problem of the error not showing ^^
But I'm still fighting to get c# working on vscode ubuntu for my unity project, if you have any hints on were to look I'll be glad to hear from you :)
I installed latest version of mono, because the error said so, and the omnisharp server started correctly but nothing was recognized. I had lot of error on System.Object and lot of other basic stuff not being recognized. It was probably my project settings that were off.

Still looking for it but I'm not sure that it is an omnisharp problem.

@DustinCampbell
Copy link
Member

I've looked through all of the launch code, and I don't think the right fix is quite what you suggest.

  • First, the promises in launcher.ts all need cleaned up. There's a lot of unnecessary junk in there.
  • Then, there should be a catch at the end of promise chain here.

Could you take a quick look at #807? I'm curious to know whether this addresses your scenario by properly allowing rejections and thrown exceptions to bubble up and properly cancel the server launch.

@DustinCampbell
Copy link
Member

There is a new beta of the C# extension available here that should address this problem. Feel free to try it out and let me know if it address the issue for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants