Differing behavior of exception in shadow endpoint vs ssr

[coryvirok]

Describe the bug

In hooks.js I wrap the await resolve(event) in a try/catch in order to do things like rollback a DB transaction.

This works fine when the exception is thrown from the shadow endpoint but the exception fails to bubble up to handle() when the same page is rendered on the server.

Reproduction

// hooks.js

export const handle = async ({ event, resolve }) => {
  let response
  try {
    response = await resolve(event);
  } catch (err) {
    console.log('@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@')
    console.log(err)
    throw err
  }
}

// routes/endpoint.js

export const get = async () => {
  throw new Error("I should see some @'s")
}

Logs

From a full page load (SSR)

I should see some @'s!
Error: I should see some @'s!
    at get (/private/tmp/my-app/src/routes/todos/index.ts:2:10)
    at load_shadow_data (file:///private/tmp/my-app/.svelte-kit/runtime/server/index.js:2530:25)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async load_node (file:///private/tmp/my-app/.svelte-kit/runtime/server/index.js:2127:5)
    at async respond$1 (file:///private/tmp/my-app/.svelte-kit/runtime/server/index.js:2804:15)
    at async resolve (file:///private/tmp/my-app/.svelte-kit/runtime/server/index.js:3309:11)
    at async Object.handle (/private/tmp/my-app/src/hooks.ts:5:13)
    at async respond (file:///private/tmp/my-app/.svelte-kit/runtime/server/index.js:3253:20)
    at async file:///private/tmp/my-app/node_modules/@sveltejs/kit/dist/chunks/index.js:294:24

From a shadow endpoint fetch (__data.json)

@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
Error: I should see some @'s!
    at get (/private/tmp/my-app/src/routes/todos/index.ts:4:9)
    at render_endpoint (file:///private/tmp/my-app/.svelte-kit/runtime/server/index.js:171:25)
    at resolve (file:///private/tmp/my-app/.svelte-kit/runtime/server/index.js:3286:24)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Object.handle (//private/tmp/my-app/src/hooks.ts:6:16)
    at async respond (file:///private/tmp/my-app/.svelte-kit/runtime/server/index.js:3253:20)
    at async file:///private/tmp/my-app/node_modules/@sveltejs/kit/dist/chunks/index.js:294:24

System Info

System:
    OS: macOS 12.2.1
    CPU: (10) arm64 Apple M1 Pro
    Memory: 240.88 MB / 32.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 17.6.0 - /opt/homebrew/bin/node
    Yarn: 1.22.18 - ~/.yarn/bin/yarn
    npm: 8.5.2 - /opt/homebrew/bin/npm
  Browsers:
    Chrome: 100.0.4896.127
    Safari: 15.3
  npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.40
    @sveltejs/kit: next => 1.0.0-next.324
    svelte: ^3.46.0 => 3.48.0

Severity

serious, but I can work around it

Additional Information

I'm not 100% sure how to work around it...

[coryvirok]

I found a viable workaround. Looks like the handleError() method always gets called regardless so I can do cleanup in there. But IMO that shouldn't be the longterm solution since control flow in handle() is now dependent on how the method was called (i.e. via __data.json or ssr).

[elliott-with-the-longest-name-on-github]

Is handleError not called in both cases?

I don't think you're supposed to be trying to catch endpoint errors from resolve (I can't find any references to doing so in the docs). It may be more of a bug that errors do bubble up to resolve than that they don't -- that's what handleError is for.

Based on a couple of things I see in those logs, I think this may fall somewhere between "rough-edged intended behavior" and "logical result of design choice" -- it looks like one of them is being handled by the router as a navigation (which could be caught with __error.svelte) and the other is being handled by the server "outside" of the router -- but I can't prove it right now, 'cause ya didn't provide a full repro and I don't have time to write yet another full Kit repro. ☹️

Post a GitHub link to a working repro and I'll take a look at it further.

[coryvirok]

Here ya go. I figured a full repo was less easy to understand. 🤷🏻

https://github.com/coryvirok/svelte-exception-bug

Relevant changes to the demo app are:

• https://github.com/coryvirok/svelte-exception-bug/blob/main/src/hooks.ts
• https://github.com/coryvirok/svelte-exception-bug/blob/main/src/routes/todos/index.ts

To repro, load the todos page directly (causes SSR to be returned.) Notice the console logs do not contain the @@@ line. Next, load the todos page by clicking on the link in the top nav, (causes the page to load via shadow endpoint.) Notice the console logs the '@@@' line.

If this is a design decision, it definitely needs some documentation since wrapping resolve(event) in a try/catch is a pretty reasonable thing to expect folks to attempt. And the fact that it works sometimes is going to add more confusion.

Lemme know if there's anything else I can provide. Thanks!

[elliott-with-the-longest-name-on-github]

Okay, check this out. I think everything that's going on here is normal behavior.

First, I don't think you're meant to catch and deal with errors caused by resolve in this way... in fact, your handle function doesn't even have the correct type. Handle must return a MaybePromise<Response>. (Fun fact, if you do what you're doing and don't rethrow the error, it will actually change the error code from a 500 to a 404.) If you type your handle correctly (import type { Handle } from '@sveltejs/kit';), it'll give you red squiggles telling you as much. Regardless, if I try to catch rendering errors from resolve, I'm catching them both on shadow endpoint requests and on page navigations

Second, if you catch the error in handle, it will not propagate to handleError.

Third, I think the strategy SvelteKit wants you to use here is to abstract your database error handling logic out into the lib folder. Internalize its error handling. Then, import that library into your endpoints and use it there, handling the errors that may occur inside the library (or in the endpoint). If there is an actual unhandled exception, it will propagate to handleError (where you can log it) and cause your nearest ancestored __error.svelte to render. Think about it: Why handle a database error caused in who-knows-what endpoint in handle? By that point, you're way past the point of being able to recover from the error and way far away from its source -- the error in handle could be caused by any number of things, your database being only one of them. If you need to be able to roll back transactions on errors, bake that behavior into your query function. :)

I did notice one bug, though, which I'll file a separate issue for...

Hopefully some other helpful souls will stop by and provide some other input, but I can't actually find any behavior with this that isn't intended.

[coryvirok]

Thanks for putting this together. I set up your repo and verified what you said about not being able to catch the error in handle(). However, if you change your index.ts endpoint to be async, you'll see that you do catch the error in handle() but only for the shadow endpoint request.

Regarding the DB transaction handling - the pattern I'm using is what I've been used to in Flask, Django, Pyramid, and other web app frameworks over the years. Creating a new transaction per request is the safest way to guarantee ACID properties for your routes.

Here's a pretty common scenario that requires transaction setup and management in the handle() function:

1. Handle is called
2. In handle() the user ID is parsed from the auth cookie
3. In handle() a DB transaction is created (and will later by rolled back or committed on successful await resolve(event)
4. In handle() the user ID is queried in the DB using the transaction and the user profile is loaded and saved to event.locals for the endpoint to handle, along with the DB transaction
5. The endpoint executes a bunch of library code that requires a transaction so the state of the DB, (REPEATABLE READ most likely) guarantees that the user profile info queried from handle() is accurate wrt the other queries being made
6. The endpoint finishes its work and returns some JSON
7. The await resolve(event) function returns successfully and the DB transaction is committed.

In the above scenario, if the transaction logic were pushed into the endpoint, there would be no way to check the auth credentials in the DB based on the auth cookie in 1 place across the entire app. Also, it would lead to tons of boilerplate try/catch in every endpoint.

Or, if 2 transactions were made, (one in handle() and the other in the endpoint) then we've broken transactionality and the app could do bad things, (like let in a user that was actually deleted between the initial auth check and the endpoint execution.)

TL;DR Is this a pattern that we want to support in SvelteKit?

The docs suggest that it is and folks are going to expect that if there's a way to wrap the call to resolve(event) that it handles errors in a straightforward and consistent way.

// hooks.js

async function handle(event) {
  event.locals.foo = 'some custom data to pass to endpoints, e.g. a DB transaction'

  // Handle any exceptions from endpoint and render
  try {
    // Execute endpoint and render
    return await resolve(event)
  } catch (err) {
    // do something, like rollback the DB transaction
    throw err
  }
}

As always, much appreciation for your time and for this project. The promise of fully server-rendered modern web apps is so, so close!

[elliott-with-the-longest-name-on-github]

Ooh, good catch on the behavior inconsistency with async methods. I wonder where that's getting lost.

Now that we've nailed down the case and had some good discussion on the details, I'll probably step back and wait for some maintainer discussion. Hopefully the info we've worked out is enough to make it easy on 'em. 🙂 It seems clear that this is one fuzzy aspect of an error handling framework that has some rough edges overall and needs some thought from an overall design perspective (there's an interesting PR referenced in the issue I filed related to this one), especially around what error handling we should be doing in handle.

[coryvirok]

Ooo, thanks for pointing me at that @tcc-sejohnson! More threads to pull on... :)

[Rich-Harris]

Without checking the repros in this thread (just doing some quick triaging for now, will circle back later), I'll just say this: await resolve(...) should never throw. If an error occurs while resolving the page/endpoint, it should invoke handleError then return a Response with the appropriate status code, meaning any cleanup can happen after the resolve.

If that's not happening, then it's a bug, but fixing it might be a breaking change for people who are relying on that bug so I've added the appropriate label.