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

Improve nodejs automation api inline error handling #6237

Merged
merged 3 commits into from
Feb 2, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 19 additions & 0 deletions sdk/nodejs/tests/automation/localWorkspace.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,25 @@ describe("LocalWorkspace", () => {
await stack.workspace.removeStack(stackName);
}
}));
it(`runs an inline program that rejects a promise and exits gracefully`, asyncTest(async () => {
const program = async () => {
Promise.reject(new Error());
return {};
};
const stackName = `int_test${getTestSuffix()}`;
const projectName = "inline_node";
const stack = await LocalWorkspace.createStack({ stackName, projectName, program });

// pulumi up
await assert.rejects(stack.up())

// pulumi destroy
const destroyRes = await stack.destroy();
assert.strictEqual(destroyRes.summary.kind, "destroy");
assert.strictEqual(destroyRes.summary.result, "succeeded");

await stack.workspace.removeStack(stackName);
}));
});

const getTestSuffix = () => {
Expand Down
43 changes: 12 additions & 31 deletions sdk/nodejs/x/automation/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,38 +27,26 @@ export const maxRPCMessageSize: number = 1024 * 1024 * 400;
/** @internal */
export class LanguageServer<T> implements grpc.UntypedServiceImplementation {
readonly program: () => Promise<T>;
readonly result: Promise<T>;

resolveResult!: (v: any) => void;
rejectResult!: (e: Error) => void;
pulumiExitCode?: number;
running: boolean;

// Satisfy the grpc.UntypedServiceImplementation interface.
[name: string]: any;

constructor(program: () => Promise<T>) {
this.program = program;
this.result = new Promise<any>((resolve, reject) => {
this.resolveResult = resolve;
this.rejectResult = reject;
});
this.running = false;
}

getExitError(preview: boolean): Error {
return new Error(this.pulumiExitCode === 0 ?
"pulumi exited prematurely" :
`${preview ? "preview" : "update"} failed with code ${this.pulumiExitCode}`);
this.running = false;
}

onPulumiExit(code: number, preview: boolean) {
this.pulumiExitCode = code;
onPulumiExit() {
// check for leaks once the CLI exits
const [leaks, leakMessage] = runtime.leakedPromises();
if (leaks.size !== 0) {
throw new Error(leakMessage);
}
// these are globals and we need to clean up after ourselves
runtime.resetOptions("", "", -1, "", "", false);
if (!this.running) {
this.rejectResult(this.getExitError(preview));
}
}

getRequiredPlugins(call: any, callback: any): void {
Expand All @@ -71,15 +59,10 @@ export class LanguageServer<T> implements grpc.UntypedServiceImplementation {
const req: any = call.request;
const resp: any = new langproto.RunResponse();

if (this.pulumiExitCode !== undefined) {
callback(this.getExitError(req.getDryrun()));
return;
}
this.running = true;

const errorSet = new Set<Error>();
const uncaughtHandler = newUncaughtHandler(errorSet);
let result: Promise<T> | undefined;
try {
const args = req.getArgsList();
const engineAddr = args && args.length > 0 ? args[0] : "";
Expand Down Expand Up @@ -117,15 +100,10 @@ export class LanguageServer<T> implements grpc.UntypedServiceImplementation {
throw new Error("One or more errors occurred");
}

const [leaks, leakMessage] = runtime.leakedPromises();
if (leaks.size !== 0) {
throw new Error(leakMessage);
}
this.resolveResult(result);
} catch (e) {
const err = e instanceof Error ? e : new Error(`unknown error ${e}`);
resp.setError(err.message);
this.rejectResult(err);
callback(err, undefined)
}

callback(undefined, resp);
Expand Down Expand Up @@ -155,7 +133,10 @@ function newUncaughtHandler(errorSet: Set<Error>): (err: Error) => void {
//
// If both the stack and message are empty, then just stringify the err object itself. This
// is also necessary as users can throw arbitrary things in JS (including non-Errors).
const defaultMessage = err.stack || err.message || ("" + err);
let defaultMessage = ""
if (!!err) {
defaultMessage = err.stack || err.message || ("" + err);
Copy link
Member

Choose a reason for hiding this comment

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

What's the relevance of the "" + err? Is that a string conversion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup

}

// First, log the error.
if (RunError.isInstance(err)) {
Expand Down
31 changes: 21 additions & 10 deletions sdk/nodejs/x/automation/stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ export class Stack {
}
}

let onExit = (code: number) => { return; };
let onExit = () => { return; };

if (program) {
kind = execKind.inline;
Expand All @@ -163,16 +163,23 @@ export class Stack {
});
});
server.start();
onExit = (code: number) => {
languageServer.onPulumiExit(code, false /* preview */);
onExit = () => {
languageServer.onPulumiExit();
server.forceShutdown();
};
args.push(`--client=127.0.0.1:${port}`);
}

args.push("--exec-kind", kind);
const upResult = await this.runPulumiCmd(args, opts?.onOutput);
onExit(upResult.code);
let upResult;
try {
upResult = await this.runPulumiCmd(args, opts?.onOutput);
onExit();
Copy link
Member

Choose a reason for hiding this comment

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

Is this onExit() necessary? Doesn't the finally block run regardless of error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. Updated.

} finally {
onExit()
}


// TODO: do this in parallel after this is fixed https://github.com/pulumi/pulumi/issues/6050
const outputs = await this.outputs();
const summary = await this.info();
Expand Down Expand Up @@ -224,7 +231,7 @@ export class Stack {
}
}

let onExit = (code: number) => { return; };
let onExit = () => { return; };

if (program) {
kind = execKind.inline;
Expand All @@ -243,16 +250,20 @@ export class Stack {
});
});
server.start();
onExit = (code: number) => {
languageServer.onPulumiExit(code, false /* preview */);
onExit = () => {
languageServer.onPulumiExit();
server.forceShutdown();
};
args.push(`--client=127.0.0.1:${port}`);
}

args.push("--exec-kind", kind);
const preResult = await this.runPulumiCmd(args);
onExit(preResult.code);
let preResult;
try {
preResult = await this.runPulumiCmd(args);
} finally {
onExit();
}
const summary = await this.info();
return {
stdout: preResult.stdout,
Expand Down