Skip to content

fix: Handle uncaught exception in create-amplify #2828

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: Handle uncaught exception in create-amplify #2828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions .changeset/eighty-spoons-tickle.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'create-amplify': patch
'@aws-amplify/cli-core': patch
'@aws-amplify/backend-cli': patch
Comment on lines +2 to +4
Copy link
Member

Choose a reason for hiding this comment

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

some of these are incorrect if we're touching public API.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would this be more accurate?

'create-amplify': patch
'@aws-amplify/cli-core': minor
'@aws-amplify/backend-cli': minor

Copy link
Member

Choose a reason for hiding this comment

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

'create-amplify': patch
'@aws-amplify/cli-core': minor
'@aws-amplify/backend-cli': patch

is the right mix.

---

Handle uncaught exception in create-amplify and refactor the error handler to cli-core
23 changes: 17 additions & 6 deletions packages/cli-core/API.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
```ts

import { AmplifyIOHost } from '@aws-amplify/plugin-types';
import { Argv } from 'yargs';
import { PackageManagerController } from '@aws-amplify/plugin-types';
import { UsageDataEmitter } from '@aws-amplify/platform-core';
import { WriteStream } from 'node:tty';
import z from 'zod';

Expand Down Expand Up @@ -33,12 +35,18 @@ export class AmplifyPrompter {
}) => Promise<boolean>;
}

// @public
export const attachUnhandledExceptionListeners: (usageDataEmitter?: UsageDataEmitter) => void;

// @public (undocumented)
export type ColorName = (typeof colorNames)[number];

// @public (undocumented)
export const colorNames: readonly ["Green", "Yellow", "Blue", "Magenta", "Cyan", "Red"];

// @public
export const extractSubCommands: (yargs: Argv) => string | undefined;

// @public
export class Format {
constructor(packageManagerRunnerName?: string);
Expand Down Expand Up @@ -75,6 +83,9 @@ export class Format {
// @public (undocumented)
export const format: Format;

// @public
export const generateCommandFailureHandler: (parser?: Argv, usageDataEmitter?: UsageDataEmitter) => ((message: string, error: Error) => Promise<void>);

// @public (undocumented)
export enum LogLevel {
// (undocumented)
Expand Down Expand Up @@ -185,7 +196,7 @@ export const noticeSchema: z.ZodObject<{
errorMessage: string;
})[];
link?: string | undefined;
frequency?: "once" | "command" | "deployment" | "daily" | undefined;
frequency?: "command" | "once" | "deployment" | "daily" | undefined;
validFrom?: number | undefined;
validTo?: number | undefined;
}, {
Expand Down Expand Up @@ -213,7 +224,7 @@ export const noticeSchema: z.ZodObject<{
errorMessage: string;
})[];
link?: string | undefined;
frequency?: "once" | "command" | "deployment" | "daily" | undefined;
frequency?: "command" | "once" | "deployment" | "daily" | undefined;
validFrom?: number | undefined;
validTo?: number | undefined;
}>;
Expand Down Expand Up @@ -314,7 +325,7 @@ export const noticesManifestSchema: z.ZodObject<{
errorMessage: string;
})[];
link?: string | undefined;
frequency?: "once" | "command" | "deployment" | "daily" | undefined;
frequency?: "command" | "once" | "deployment" | "daily" | undefined;
validFrom?: number | undefined;
validTo?: number | undefined;
}, {
Expand Down Expand Up @@ -342,7 +353,7 @@ export const noticesManifestSchema: z.ZodObject<{
errorMessage: string;
})[];
link?: string | undefined;
frequency?: "once" | "command" | "deployment" | "daily" | undefined;
frequency?: "command" | "once" | "deployment" | "daily" | undefined;
validFrom?: number | undefined;
validTo?: number | undefined;
}>, "many">;
Expand Down Expand Up @@ -372,7 +383,7 @@ export const noticesManifestSchema: z.ZodObject<{
errorMessage: string;
})[];
link?: string | undefined;
frequency?: "once" | "command" | "deployment" | "daily" | undefined;
frequency?: "command" | "once" | "deployment" | "daily" | undefined;
validFrom?: number | undefined;
validTo?: number | undefined;
}[];
Expand Down Expand Up @@ -402,7 +413,7 @@ export const noticesManifestSchema: z.ZodObject<{
errorMessage: string;
})[];
link?: string | undefined;
frequency?: "once" | "command" | "deployment" | "daily" | undefined;
frequency?: "command" | "once" | "deployment" | "daily" | undefined;
validFrom?: number | undefined;
validTo?: number | undefined;
}[];
Expand Down
3 changes: 2 additions & 1 deletion packages/cli-core/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"strip-ansi": "^7.1.0",
"wrap-ansi": "^9.0.0",
"semver": "^7.6.3",
"zod": "3.25.17"
"zod": "3.25.17",
"yargs": "^17.7.2"
}
}
4 changes: 3 additions & 1 deletion packages/cli/src/error_handler.test.ts → packages/cli-core/src/error_handler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ import {
generateCommandFailureHandler,
} from './error_handler.js';
import { Argv } from 'yargs';
import { LogLevel, format, printer } from '@aws-amplify/cli-core';
import { format } from './format/format.js';
import { LogLevel } from './printer/printer.js';
import { printer } from './printer.js';
import assert from 'node:assert';
import { AmplifyUserError, UsageDataEmitter } from '@aws-amplify/platform-core';

Expand Down
44 changes: 28 additions & 16 deletions packages/cli/src/error_handler.ts → packages/cli-core/src/error_handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { LogLevel, format, printer } from '@aws-amplify/cli-core';
import { format } from './format/format.js';
import { LogLevel } from './printer/printer.js';
import { printer } from './printer.js';
import { Argv } from 'yargs';
import { AmplifyError, UsageDataEmitter } from '@aws-amplify/platform-core';
import { extractSubCommands } from './extract_sub_commands.js';
Expand All @@ -17,7 +19,7 @@ type HandleErrorProps = {
* Attaches process listeners to handle unhandled exceptions and rejections
*/
export const attachUnhandledExceptionListeners = (
usageDataEmitter: UsageDataEmitter,
usageDataEmitter?: UsageDataEmitter,
): void => {
if (hasAttachUnhandledExceptionListenersBeenCalled) {
return;
Expand Down Expand Up @@ -54,7 +56,7 @@ export const attachUnhandledExceptionListeners = (
* This prevents our top-level error handler from being invoked after the yargs error handler has already been invoked
*/
export const generateCommandFailureHandler = (
parser: Argv,
parser?: Argv,
usageDataEmitter?: UsageDataEmitter,
): ((message: string, error: Error) => Promise<void>) => {
/**
Expand All @@ -63,19 +65,29 @@ export const generateCommandFailureHandler = (
* @param error error thrown by yargs handler
*/
const handleCommandFailure = async (message: string, error?: Error) => {
const printHelp = () => {
printer.printNewLine();
parser.showHelp();
printer.printNewLine();
};
await handleErrorSafe({
command: extractSubCommands(parser),
printMessagePreamble: printHelp,
error,
message,
usageDataEmitter,
});
parser.exit(1, error || new Error(message));
if (!parser) {
await handleErrorSafe({
error,
message,
});
}

// for ampx commands
if (parser) {
const printHelp = () => {
printer.printNewLine();
parser.showHelp();
printer.printNewLine();
};
await handleErrorSafe({
command: extractSubCommands(parser),
printMessagePreamble: printHelp,
error,
message,
usageDataEmitter,
});
parser.exit(1, error || new Error(message));
}
};
return handleCommandFailure;
};
Expand Down
0 packages/cli/src/extract_sub_commands.ts → ...ages/cli-core/src/extract_sub_commands.ts
View file
Open in desktop
File renamed without changes.
2 changes: 2 additions & 0 deletions packages/cli-core/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,5 @@ export * from './package-manager-controller/package_manager_controller_factory.j
export * from './loggers/amplify_io_events_bridge_singleton_factory.js';
export * from './notices/notices.js';
export * from './notices/notices_manifest_validator.js';
export * from './error_handler.js';
export * from './extract_sub_commands.js';
13 changes: 7 additions & 6 deletions packages/cli/src/ampx.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,6 @@ import {
import { AsyncLocalStorageContextManager } from '@opentelemetry/context-async-hooks';
import { BasicTracerProvider } from '@opentelemetry/sdk-trace-base';
import { createMainParser } from './main_parser_factory.js';
import {
attachUnhandledExceptionListeners,
generateCommandFailureHandler,
} from './error_handler.js';
import { extractSubCommands } from './extract_sub_commands.js';
import {
AmplifyFault,
PackageJsonReader,
Expand All @@ -25,7 +20,13 @@ import {
import { fileURLToPath } from 'node:url';
import { verifyCommandName } from './verify_command_name.js';
import { hideBin } from 'yargs/helpers';
import { PackageManagerControllerFactory, format } from '@aws-amplify/cli-core';
import {
PackageManagerControllerFactory,
attachUnhandledExceptionListeners,
extractSubCommands,
format,
generateCommandFailureHandler,
} from '@aws-amplify/cli-core';
import { NoticesRenderer } from './notices/notices_renderer.js';
import { extractCommandInfo } from './extract_command_info.js';
import { DeepPartial } from '@aws-amplify/plugin-types';
Expand Down
6 changes: 4 additions & 2 deletions packages/cli/src/test-utils/command_runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import { Argv } from 'yargs';
import { AsyncLocalStorage } from 'node:async_hooks';
import { UsageDataEmitter } from '@aws-amplify/platform-core';
import { generateCommandFailureHandler } from '../error_handler.js';
import { extractSubCommands } from '../extract_sub_commands.js';
import {
extractSubCommands,
generateCommandFailureHandler,
} from '@aws-amplify/cli-core';

class OutputInterceptor {
private output = '';
Expand Down
40 changes: 22 additions & 18 deletions packages/create-amplify/src/create_amplify.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,37 +8,41 @@
*/

import {
LogLevel,
PackageManagerControllerFactory,
attachUnhandledExceptionListeners,
format,
printer,
generateCommandFailureHandler,
} from '@aws-amplify/cli-core';
import { ProjectRootValidator } from './project_root_validator.js';
import { AmplifyProjectCreator } from './amplify_project_creator.js';
import { getProjectRoot } from './get_project_root.js';
import { GitIgnoreInitializer } from './gitignore_initializer.js';
import { InitialProjectFileGenerator } from './initial_project_file_generator.js';

const projectRoot = await getProjectRoot();
attachUnhandledExceptionListeners();
const errorHandler = generateCommandFailureHandler();

const packageManagerControllerFactory = new PackageManagerControllerFactory(
projectRoot,
);
try {
const projectRoot = await getProjectRoot();

const packageManagerController =
packageManagerControllerFactory.getPackageManagerController();
const packageManagerControllerFactory = new PackageManagerControllerFactory(
projectRoot,
);

const amplifyProjectCreator = new AmplifyProjectCreator(
projectRoot,
packageManagerController,
new ProjectRootValidator(projectRoot),
new GitIgnoreInitializer(projectRoot),
new InitialProjectFileGenerator(projectRoot, packageManagerController),
);
const packageManagerController =
packageManagerControllerFactory.getPackageManagerController();

const amplifyProjectCreator = new AmplifyProjectCreator(
projectRoot,
packageManagerController,
new ProjectRootValidator(projectRoot),
new GitIgnoreInitializer(projectRoot),
new InitialProjectFileGenerator(projectRoot, packageManagerController),
);

try {
await amplifyProjectCreator.create();
} catch (err) {
printer.log(format.error(err), LogLevel.ERROR);
process.exitCode = 1;
if (err instanceof Error) {
await errorHandler(format.error(err), err);
}
Comment on lines +45 to +47
Copy link
Member

Choose a reason for hiding this comment

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

With this implementation we'll be blind to non-Errors.
We should handle them some way.
Can you please check how this is handled in other places?

Copy link
Member Author

Choose a reason for hiding this comment

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

Forgive my ignorance as I'm new to the code base, but isn't that handled by the attachUnhandledExceptionListeners?

Copy link
Member

Choose a reason for hiding this comment

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

No. once err is caught (by catch block) it's considered handled unless re-thrown.

You can assert this by adding throw 'foo' somewhere in try block and see what happens.

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha, I see that now thanks! I took a look around and it looks like largely, the downstream instances will wrap the non-Errors into an AmplifyError or some sort and then shoot it upstream. Meanwhile, in ampx, it only catches instances of Errors only.

So I could keep the original code that was here alongisde the handling of Error like so:

} catch (err) {
  if (err instanceof Error) {
    await errorHandler(format.error(err), err);
  } else {
    printer.log(format.error(err), LogLevel.ERROR);
    process.exitCode = 1;
  }
}

which shows up like so:

❯ npx create-amplify
2:51:22 PM [ERROR] foo

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I do wonder if we can just remove the try-catch from here and just let it bubble up to "UnhandledExceptionListener" that we also introduce in this PR. It's implementation seems to be dealing with non-Errors.

Copy link
Member

Choose a reason for hiding this comment

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

@Amplifiyer might have some ideas here as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

So it looks like removing the try-catch and solely using the UnhandledExceptionListener doesn't work the same as what you wanted for the throw 'foo' scenario. This method in the error handler throws the exception:

TypeError: Cannot read properties of undefined (reading 'includes')

Becase it's a non-Error it sounds like the expected behavior (as per the code) of is to just print a Debug-level log and then gracefully exit.

}
Loading