Skip to content

Don't immediately return promise within newly created promise #807

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Oct 11, 2016
Merged

Don't immediately return promise within newly created promise #807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
875e3a6
Don't immediately return promise within newly created promise
DustinCampbell Oct 10, 2016
a880c20
Further clean up of OmniSharp launcher promises
DustinCampbell Oct 11, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 54 additions & 67 deletions src/omnisharp/launcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,21 +161,15 @@ export interface LaunchResult {

export function launchOmniSharp(details: LaunchDetails): Promise<LaunchResult> {
return new Promise<LaunchResult>((resolve, reject) => {
try {
return 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);
}, err => {
reject(err);
});
} catch (err) {
reject(err);
}
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);
});
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.

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

Copy link
Member Author

@DustinCampbell DustinCampbell Oct 12, 2016

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 •
edited
Loading

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);

});
}

Expand All @@ -189,34 +183,31 @@ function launch(details: LaunchDetails): Promise<LaunchResult> {
}

function launchWindows(details: LaunchDetails): Promise<LaunchResult> {
return new Promise<LaunchResult>(resolve => {

function escapeIfNeeded(arg: string) {
const hasSpaceWithoutQuotes = /^[^"].* .*[^"]/;
return hasSpaceWithoutQuotes.test(arg)
? `"${arg}"`
: arg;
}
function escapeIfNeeded(arg: string) {
const hasSpaceWithoutQuotes = /^[^"].* .*[^"]/;
return hasSpaceWithoutQuotes.test(arg)
? `"${arg}"`
: arg;
}

let args = details.args.slice(0); // create copy of details.args
args.unshift(details.serverPath);
args = [[
'/s',
'/c',
'"' + args.map(escapeIfNeeded).join(' ') + '"'
].join(' ')];

let process = spawn('cmd', args, <any>{
windowsVerbatimArguments: true,
detached: false,
cwd: details.cwd
});
let args = details.args.slice(0); // create copy of details.args
args.unshift(details.serverPath);
args = [[
'/s',
'/c',
'"' + args.map(escapeIfNeeded).join(' ') + '"'
].join(' ')];

let process = spawn('cmd', args, <any>{
windowsVerbatimArguments: true,
detached: false,
cwd: details.cwd
});

return resolve({
process,
command: details.serverPath,
usingMono: false
});
return Promise.resolve({
process,
command: details.serverPath,
usingMono: false
});
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.

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

@DustinCampbell DustinCampbell Oct 12, 2016

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.

}

Expand All @@ -233,48 +224,44 @@ function launchNix(details: LaunchDetails): Promise<LaunchResult> {
}

function launchNixCoreCLR(details: LaunchDetails): Promise<LaunchResult> {
return new Promise<LaunchResult>(resolve => {
let process = spawn(details.serverPath, details.args, {
detached: false,
cwd: details.cwd
});
let process = spawn(details.serverPath, details.args, {
detached: false,
cwd: details.cwd
});

return resolve({
process,
command: details.serverPath,
usingMono: false
});
return Promise.resolve({
process,
command: details.serverPath,
usingMono: false
});
}

function launchNixMono(details: LaunchDetails): Promise<LaunchResult> {
return new Promise<LaunchResult>((resolve, reject) => {
return canLaunchMono().then(() => {
let args = details.args.slice(0); // create copy of details.args
args.unshift(details.serverPath);

let process = spawn('mono', args, {
detached: false,
cwd: details.cwd
});
return canLaunchMono().then(() => {
let args = details.args.slice(0); // create copy of details.args
args.unshift(details.serverPath);

return resolve({
process,
command: details.serverPath,
usingMono: true
});
let process = spawn('mono', args, {
detached: false,
cwd: details.cwd
});

return {
process,
command: details.serverPath,
usingMono: true
};
});
}

function canLaunchMono(): Promise<void> {
return new Promise<void>((resolve, reject) => {
hasMono('>=4.0.1').then(success => {
if (success) {
return resolve();
resolve();
}
else {
return reject(new Error('Cannot start Omnisharp because Mono version >=4.0.1 is required.'));
reject(new Error('Cannot start Omnisharp because Mono version >=4.0.1 is required.'));
}
});
});
Expand Down
16 changes: 7 additions & 9 deletions src/omnisharp/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,10 +299,9 @@ export abstract class OmnisharpServer {
this._telemetryIntervalId = setInterval(() => this._reportTelemetry(), TelemetryReportingDelay);
}).then(() => {
this._processQueue();
}, err => {
}).catch(err => {
this._fireEvent(Events.ServerError, err);
this._setState(ServerState.Stopped);
throw err;
return this.stop();
});
});
}
Expand All @@ -311,7 +310,7 @@ export abstract class OmnisharpServer {

public stop(): Promise<void> {

let ret: Promise<void>;
let cleanupPromise: Promise<void>;

if (this._telemetryIntervalId !== undefined) {
// Stop reporting telemetry
Expand All @@ -322,13 +321,13 @@ export abstract class OmnisharpServer {

if (!this._serverProcess) {
// nothing to kill
ret = Promise.resolve();
cleanupPromise = Promise.resolve();
}
else if (process.platform === 'win32') {
// when killing a process in windows its child
// processes are *not* killed but become root
// processes. Therefore we use TASKKILL.EXE
ret = new Promise<void>((resolve, reject) => {
cleanupPromise = new Promise<void>((resolve, reject) => {
const killer = exec(`taskkill /F /T /PID ${this._serverProcess.pid}`, (err, stdout, stderr) => {
if (err) {
return reject(err);
Expand All @@ -342,14 +341,13 @@ export abstract class OmnisharpServer {
else {
// Kill Unix process
this._serverProcess.kill('SIGTERM');
ret = Promise.resolve();
cleanupPromise = Promise.resolve();
}

return ret.then(() => {
return cleanupPromise.then(() => {
this._serverProcess = null;
this._setState(ServerState.Stopped);
this._fireEvent(Events.ServerStop, this);
return;
});
}

Expand Down