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.

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

Handle unhandledrejections in the dev server #9424

Merged
merged 6 commits into from
Dec 14, 2023
Merged

Handle unhandledrejections in the dev server #9424

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
da46bc1
Handle unhandledrejections in the dev server
matthewp Dec 13, 2023
2d9b88e
Adding changeset
matthewp Dec 13, 2023
a550e92
Update .changeset/curvy-lobsters-crash.md
matthewp Dec 13, 2023
8693171
Use AsyncLocalStorage
matthewp Dec 13, 2023
46222e9
Return errorWithMetadata
matthewp Dec 13, 2023
9b9f51b
Send the error to the browser
matthewp Dec 13, 2023
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
5 changes: 5 additions & 0 deletions .changeset/curvy-lobsters-crash.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Prevent dev server from crashing on unhandled rejections
9 changes: 9 additions & 0 deletions packages/astro/src/core/errors/errors-data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1292,3 +1292,12 @@ export const CantRenderPage = {

// Generic catch-all - Only use this in extreme cases, like if there was a cosmic ray bit flip
export const UnknownError = { name: 'UnknownError', title: 'Unknown Error.' } satisfies ErrorData;

export const UnhandledRejection = {
name: 'UnhandledRejection',
title: 'Unhandled rejection',
message: (stack: string) => {
return `Astro detected an unhandled rejection. Here's the stack trace:\n${stack}`;
},
hint: 'Make sure your promises all have an `await` or a `.catch()` handler.'
}
11 changes: 11 additions & 0 deletions packages/astro/src/vite-plugin-astro-server/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { baseMiddleware } from './base.js';
import { createController } from './controller.js';
import DevPipeline from './devPipeline.js';
import { handleRequest } from './request.js';
import { AstroError, AstroErrorData } from '../core/errors/index.js';

export interface AstroPluginOptions {
settings: AstroSettings;
Expand Down Expand Up @@ -43,6 +44,16 @@ export default function createVitePluginAstroServer({
viteServer.watcher.on('unlink', rebuildManifest.bind(null, true));
viteServer.watcher.on('change', rebuildManifest.bind(null, false));

function handleUnhandledRejection(rejection: any) {
const error = new AstroError({
...AstroErrorData.UnhandledRejection,
message: AstroErrorData.UnhandledRejection.message(rejection?.stack || rejection)
})
logger.error(null, error.message);
}
Copy link
Member

@Princesseuh Princesseuh Dec 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It's a bit of a edge case, but presumably this should pass through our normal-ish error pipeline so we get a nicely formatted error.

astro/packages/astro/src/core/messages.ts

Line 257 in f8b27b0

export function formatErrorMessage(err: ErrorWithMetadata, showFullStacktrace: boolean): string {

How does this look in the browser too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can't pass through the normal error pipeline, it happens outside of the context of a request. You could do: setTimeout(() => Promise.reject('foo'), 5000) and there is no ongoing request, the rejection still happens.

It doesn't currently show up in the browser, do we have a way to send an error from the server to the client outside of a request?

Copy link
Member

Choose a reason for hiding this comment

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

I would think that from where you are in the code, you'd be able to send to the websocket since you're so close to the server creation, here's how it works in the rendering:

astro/packages/astro/src/vite-plugin-astro-server/response.ts

Line 32 in f8b27b0

setTimeout(async () => loader.webSocketSend(await getViteErrorPayload(err)), 200)

You should just need to send the error payload through the websocket and it should work!»

It can't pass through the normal error pipeline, it happens outside of the context of a request.

If you can't make it go through the entire pipeline, at least formatting the error message would be great:

astro/packages/astro/src/core/messages.ts

Line 257 in f8b27b0

export function formatErrorMessage(err: ErrorWithMetadata, showFullStacktrace: boolean): string {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm looking to see if async_hooks can help. https://nodejs.org/api/async_hooks.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think AsyncLocalStorage can maybe allow this, going to give that try.

Copy link
Member

Choose a reason for hiding this comment

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

And so it begins... I have many ideas for AsyncLocalStorage!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR and it should work now. Updated with a screenshot of what the browser receives. Would appreciate another review @Princesseuh


process.on('unhandledRejection', handleUnhandledRejection);

return () => {
// Push this middleware to the front of the stack so that it can intercept responses.
// fix(#6067): always inject this to ensure zombie base handling is killed after restarts
Expand Down