Dev server crashes on thrown exception in async function

genericFJS commented 2 years ago

Describe the bug

When an error is thrown in an async function the dev server crashes with the static adapter enabled. With no adapter, the dev server works just fine. The static build seems to work just fine as well.

Reproduction

Create new sveltekit skeleton project

Add code similar to the solution from https://svelte.dev/tutorial/await-blocks to index.svelte:


<script>
async function getRandomNumber() {
await new Promise((resolve) => {
  setTimeout(() => resolve(), 1000);
});
const randomNumber = Math.random();

if (randomNumber > 0.5) {
  return randomNumber;
} else {
  throw new Error(randomNumber.toString());
}
}

let promise = getRandomNumber();

function handleClick() {
promise = getRandomNumber();
}
</script>

{#await promise}

...waiting

{:then number}

The number is {number}

{:catch error}

{error.message}

{/await}

- Execute `npm run dev` and open browser: everything works fine
- Install static adapter and add it in `svelte.config.js`
- Execute `npm run dev` and open browser: dev server crashes with error ⚡
- Execute `npm run build` and open build on webserver: everthing seems to work fine
- Execute `npm run preview` after build: preview server crashes with error as well ⚡

### Logs

```shell
PS C:\Temp\sveltekit> npm run dev

> sveltekit@0.0.1 dev
> svelte-kit dev

Pre-bundling dependencies:
  svelte/store
  svelte
  svelte/animate
  svelte/easing
  svelte/internal
  (...and 2 more)
(this will be run only when your dependencies or config have changed)
  vite v2.5.10 dev server running at:

  > Local: http://localhost:3000/
  > Network: use `--host` to expose

  SvelteKit v1.0.0-next.176

  network: not exposed
  network: not exposed
  network: not exposed
  network: not exposed
  local:   http://localhost:3000
  network: not exposed
  network: not exposed
  network: not exposed

  Use --host to expose server to other devices on this network

/src/routes/index.svelte:17
                throw new Error(no.toString());
                      ^

Error: 0.3690497328470541
    at getRandomNumber (/src/routes/index.svelte:17:9)

System Info

System:
    OS: Windows 10 10.0.19042
    CPU: (16) x64 AMD Ryzen 7 3700X 8-Core Processor
    Memory: 23.05 GB / 31.95 GB
  Binaries:
    Node: 16.4.1 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.4 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
    npm: 7.19.1 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Chrome: 94.0.4606.61
    Edge: Spartan (44.19041.1023.0), Chromium (94.0.992.31)
  npmPackages:
    @sveltejs/adapter-static: ^1.0.0-next.20 => 1.0.0-next.20
    @sveltejs/kit: next => 1.0.0-next.176
    svelte: ^3.42.6 => 3.43.0

Severity

annoyance

Additional Information

No response

benmccann commented 2 years ago

Execute npm run dev and open browser: everything works fine

Install static adapter and add it in svelte.config.js

Execute npm run dev and open browser: dev server crashes with error ⚡

This series of steps seems implausible to me. It's extremely unlikely that installing any adapter would affect dev mode. Rather, I expect what's happening is that because your example depends on Math.random it's triggered only sometimes and whether it was triggered or not was completely independent of whether the static adapter was installed.

benmccann commented 2 years ago

I'm closing this since the issue reporting guidelines, which are very explicit about the requirement for a reproduction repo, were not followed

genericFJS commented 2 years ago

@benmccann You were right, Math.random made the error occour only sometimes. The error does not seem to be with the static adapter. I made a repository and narrowed the bug down: https://github.com/toolongformore/sveltekit-adapter-static-bug (sorry for forgetting to include one in the original report) The problem arises, when a variable is initialized with an error-throwing promise; an error thrown during runtime poses no problem:

<script lang="ts">
  async function getRandomNumber(throwError: boolean) {
    if (throwError) {
      throw new Error('error');
    } else {
      return 'ok';
    }
  }

  // with this line, everything works fine
  let promise = getRandomNumber(false);
  // with this line, dev server crashes on start
  // let promise = getRandomNumber(true);
</script>

benmccann commented 2 years ago

And here's the error log:

(node:729478) UnhandledPromiseRejectionWarning: Error: error
    at eval (/src/routes/index.svelte:47:11)
    at Generator.next (<anonymous>)
    at eval (/src/routes/index.svelte:40:67)
    at new Promise (<anonymous>)
    at __awaiter (/src/routes/index.svelte:17:10)
    at getRandomNumber (/src/routes/index.svelte:45:10)
    at eval (/src/routes/index.svelte:57:16)
    at Object.$$render (/node_modules/svelte/internal/index.js:1684:22)
    at Object.default (/.svelte-kit/dev/generated/root.svelte:57:127)
    at eval (/.svelte-kit/dev/components/layout.svelte:8:41)
(Use `node --trace-warnings ...` to show where the warning was created)
(node:729478) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:729478) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Like it says, you should wrap the code in the promise with a try-catch or add a {:catch error} to your {#await} block

Perhaps SvelteKit could use this: https://nodejs.org/api/process.html#process_event_unhandledrejection

benmccann commented 2 years ago

Hmm. I'm not quite sure how to listen for unhandledRejection in SvelteKit. We wouldn't have the request available if we use the process.on method, so you couldn't include the logged in user when reporting to Sentry, etc.

I tried to put a try/catch around the ssrLoadModule call, but it didn't have any effect, which I think is expected, but I'm not a pro with promise error handling

genericFJS commented 2 years ago

Like it says, you should wrap the code in the promise with a try-catch or add a {:catch error} to your {#await} block

The given example in the repository has a {:catch error} block, this should not be the problem:

{#await promise}
  <p>...waiting</p>
{:then number}
  <p>The number is {number}</p>
{:catch error}
  <p style="color: red">{error.message}</p>
{/await}

Interestingly if I assign the promise in onMount, everything is fine as well. There only seems to be a problem, when the rejecting promise is assigned at declaration:

// crashes:
let promise = getRandomNumber(true);
// works:
let promise;
onMount(()=> { promise = getRandomNumber(true); });

Conduitry commented 2 years ago

For a component like

{#await Promise.reject('foo')}
    a
{:then}
    b
{:catch}
    c
{/await}

the compiler in SSR mode currently produces

/* App.svelte generated by Svelte v3.43.0 */
import { create_ssr_component, is_promise } from "svelte/internal";

const App = create_ssr_component(($$result, $$props, $$bindings, slots) => {
    return `${(function (__value) {
        if (is_promise(__value)) return `
    a
`;

        return (function () {
            return `
    b
`;
        })(__value);
    })(Promise.reject('foo'))}`;
});

export default App;

This crashes Node 16 because of the unhandled rejected promise.

If we wanted the compiled SSR code to always suppress this (by appeasing Node by making it see the promise as handled) we could make this change in the compiler:

diff --git a/src/compiler/compile/render_ssr/handlers/AwaitBlock.ts b/src/compiler/compile/render_ssr/handlers/AwaitBlock.ts
index 6a62f872b..c8b0eeb38 100644
--- a/src/compiler/compile/render_ssr/handlers/AwaitBlock.ts
+++ b/src/compiler/compile/render_ssr/handlers/AwaitBlock.ts
@@ -13,7 +13,10 @@ export default function(node: AwaitBlock, renderer: Renderer, options: RenderOpt

    renderer.add_expression(x`
        function(__value) {
-           if (@is_promise(__value)) return ${pending};
+           if (@is_promise(__value)) {
+               __value.catch(() => {});
+               return ${pending};
+           }
            return (function(${node.then_node ? node.then_node : ''}) { return ${then}; }(__value));
        }(${node.expression.node})
    `);

Or, it might be a good idea to only add this __value.catch(() => {}); when there's a catch on the {#await}. I'm not sure.

Conduitry commented 2 years ago

If we wanted to do that,

diff --git a/src/compiler/compile/render_ssr/handlers/AwaitBlock.ts b/src/compiler/compile/render_ssr/handlers/AwaitBlock.ts
index 6a62f872b..36eecf38c 100644
--- a/src/compiler/compile/render_ssr/handlers/AwaitBlock.ts
+++ b/src/compiler/compile/render_ssr/handlers/AwaitBlock.ts
@@ -1,6 +1,6 @@
 import Renderer, { RenderOptions } from '../Renderer';
 import AwaitBlock from '../../nodes/AwaitBlock';
-import { x } from 'code-red';
+import { b, x } from 'code-red';

 export default function(node: AwaitBlock, renderer: Renderer, options: RenderOptions) {
    renderer.push();
@@ -13,7 +13,10 @@ export default function(node: AwaitBlock, renderer: Renderer, options: RenderOpt

    renderer.add_expression(x`
        function(__value) {
-           if (@is_promise(__value)) return ${pending};
+           if (@is_promise(__value)) {
+               ${node.catch_node != null && b`__value.catch(() => {});`}
+               return ${pending};
+           }
            return (function(${node.then_node ? node.then_node : ''}) { return ${then}; }(__value));
        }(${node.expression.node})
    `);

would only suppress the rejected promise if there was a catch block, and would otherwise let it crash the process if it weren't handled some other way like with process.on.

Conduitry commented 2 years ago

I've created https://github.com/sveltejs/svelte/issues/6789 for the Svelte side of this. We can continue the discussion here about how to deal with other types of unhandled rejections, and whether that ought to be the framework's job or the user's job.

Rich-Harris commented 2 years ago

If the framework were to try and handle it, it would have to be in coordination with adapters, since process.on only makes sense in Node. In order for handleError to capture these errors we'd need to expose it as server.handleError or something (need to think about how this would work with scoped hooks/middleware — perhaps handleError can only be implemented in the root hook/middleware), and we wouldn't be able to associate it with an event

Conduitry commented 2 years ago

Do we have any sort of an idea of what other serverless hosting environments do when there's an unhandled rejection, and whether they let user code hook into that?

Rich-Harris commented 2 years ago

I have no idea, honestly

safwansamsudeen commented 9 months ago

I'm facing this now, and it's supremely annoying - any update, @Rich-Harris, @Conduitry?

sveltejs / kit