Enable top-level wrapping of all user (i.e. not Kit) code

[coryvirok]

Describe the problem

I've been struggling with a way to wrap my endpoint code in such a way that any exception thrown will be handled by a single piece of logic in order to clean up various resources after an endpoint executes.

Some things I've tried:

1. wrap await resolve(event) in hooks.ts handle() function
2. create a wrapper for every endpoint export that wraps the call to the endpoint function

First approach: Top-level resolve(event) wrapper

The first doesn't work due to strange and unsupported exception dynamics. E.g. sometimes the try/catch will catch the error and sometimes it won't. See #4801 for more details. Also, @Rich-Harris suggests to use a wrapper function - #4712 (comment). Hence the second approach.

Second approach: Endpoint wrappers

The second option isn't working because of the timing of the call to getSession(). This one is a bit more nuanced and I don't see a way around fixing it with the current design.

Let's take a step back and describe how a request should be fulfilled:

1. request comes into the server
2. an auth token is found and a user ID is parsed out of it
3. since we have a request that looks like it should be authenticated, (due to the presence of the userId) we query the database to make sure this is in fact a valid request. E.g. we check to see if the user hasn't been deleted.
4. we pass the user and the database transaction set up to fetch the user to the endpoint so it can execute all of its logic within the same DB transaction. This is important because we don't want to create a race condition by using 2 different transactions to capture the auth info + the endpoint's queries.
5. the endpoint runs and throws an Error for whatever reason, which causes the endpoint wrapper to catch the error and rolls the transaction back before re-throwing the error so that SvelteKit can handle it in whatever manner it needs to.

Now, the issue I mentioned with getSession() is that this is the thing that needs to look the user up in the DB in order to pass the user object to the browser. Which means it's the thing that needs to create the DB transaction. But since it is called before the endpoint wrapper code that has the try/catch logic, it will not be able to manage failure cases that require a rollback.

Fwiw, I tried moving the logic out of getSession() that fetches from the DB and put it into the endpoint wrapper. But apparently the session object from $app/stores is read-only on the server which means I wasn't able to modify the session before the endpoint had access to it.

---

So, the fundamental question is - How do you wrap all of your domain logic in a try/catch? This includes getSession() and every line of code in your endpoints and ideally all of the rendering for templates on the server.

Describe the proposed solution

Listed in order of effort I think would be required to implement (high to low.)

Option 1: Simplify handle() so every exception bubbles up to it

IMO, the most straightforward and elegant way to achieve this is to support it in the handle() function. This requires that exceptions thrown by endpoints or getSession(), (and I'm assuming load() as well) bubble up to handle() in a consistent way. (See the description above for examples of when that doesn't happen.)

Option 2: Provide a wrapper method for endpoints that is called before getSession() or is provided a way to mutate the session before executing the wrapped endpoint.

In this option, there is some sort of SvelteKit provided "wrap my endpoint" method that is called by resolve(event) inside handle(). This wrapper would be able to implement a try/catch around all endpoint + getSession() logic. Or, it would be provided the result of getSession() and be able to mutate it before calling the endpoint or rendering the template.

Option 3: Enable server-side mutation of the session object from $app/stores

This is probably the quickest way to implement what I'm looking for. It would allow all domain logic, (i.e. the code I write) to be wrapped by endpoint wrappers, (i.e. the second approach in the description I tried and the one proposed by @Rich-Harris.) Leaving getSession() to almost be a no-op since the wrappers or endpoint could update the session store as they saw fit.

I'm sure there are dragons with this approach...

Alternatives considered

1. wrap await resolve(event) in hooks.ts handle() function
2. create a wrapper for every endpoint export that wraps the call to the endpoint function

Importance

i cannot use SvelteKit without it

Additional Information

Thanks to all of the creators and maintainers on this project. Seriously, it's magical and I can't wait to get it into production!

[benmccann]

Have you seen handleError? https://kit.svelte.dev/docs/hooks#handleerror

You could stick a reference to anything you need on event.locals so that you can then clean it up in that method

[coryvirok]

Hey Ben. Thanks and ya I have. The problem with using handleError() to clean up is if you do, then you essentially need to put the setup code (e.g. creating the DB transaction) in the handle() function. And if you do that, you need to wrap resolve(event) in a try/catch. If you don't wrap it in a try/catch, you have to handle a bunch of corner cases.

e.g. Here's what it would look like to do the error handling in handle() + handleError() given the current semantics of exceptions in resolve()

// hooks.ts

export async function handle({event, resolve}) {
    // setup txn
    const txn = await beginTransaction()
    event.locals.txn = txn

    // call endpoints/renderers 
    // sometimes this swallows exceptions and other times it doesn't
    const resp = await resolve(event)

    // commit... but only if resolve doesn't throw an error, causing this line to never execute.
    // but resolve() might not throw an error and instead return status: 500 (unexpected error not handled
    // by app logic), which should cause a rollback
    if (resp.ok) {    
        await txn.commit()
    } else if (resp.status === 500) {
        await txn.rollback()
    }
    // set txn to null because if there was an exception swallowed by `resolve()` `handleError()` is going to be
    // called right after this function which means we don't want to attempt to rollback twice.
    event.locals.txn = null
}

export async function handleError({error, event}) {
    if (event.locals.txn) {
        event.locals.txn.rollback()

        // set this to null just in case `handleError()` is called before `resolve(event)` returns... which I think might
        // happen when loading nested nodes.
        event.locals.txn = null
    }
}

[coryvirok]

I've been chatting with folks in the Discord, and they seem to think that getSession() should be called after the endpoint. That's not the behavior I'm seeing. Here's a repro repo - https://github.com/coryvirok/svelte-getsession-order-bug

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

Personally, I'd prefer #1 -- it would be nice to be able to be able to catch all errors via handle while there's still a chance to redirect users if there's an issue.

[coryvirok]

Option #2 being asked for here - #3912

[Rich-Harris]

This behaviour may have changed since the issue was opened, but: resolve will never throw. If an error occurs, the response will be an error response. That means you can always roll stuff back inside handle. Additionally, there's no more getSession, further simplifying things, so I'll close this issue.

[coryvirok]

Awesome. Thanks Rich