Skip to content

Conversation

@DustinCampbell
Copy link
Member

Fixes #804

@DustinCampbell DustinCampbell added this to the 1.5 milestone Oct 10, 2016
setTimeout(function () {
resolve(result);
}, 0);
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it normal that rejection of launch promise is not handled here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's intended that rejection will be propagated to the caller, which is in server.ts here.

Copy link

@narf narf Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no reject callback AND the "launch" promise is not returned anywhere, I'm quite sure that the error will be silent.

This should be the correct way to bubble the error to the caller:

launch(details).then(result => {
    // async error - when target not not ENEOT
    result.process.on('error', reject);

    // success after a short freeing event loop
    setTimeout(function () {
        resolve(result);
    }, 0);
}).catch(reject);

process,
command: details.serverPath,
usingMono: false
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not putting all the code in a Promise like previously?
Any error there will end up in a throw which is not really what a function returning a promise should do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any exceptions should ultimately be propagated to the caller in server.ts. The catch() there handles all rejections and exceptions.

@DustinCampbell DustinCampbell deleted the fix-omnisharp-launchNix branch October 25, 2016 16:26
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