kit icon indicating copy to clipboard operation
kit copied to clipboard

Rejecting streamed data

Open rafaberaldo opened this issue 1 year ago • 22 comments

Describe the bug

When rejecting promises for streamed data, dev server usually crashes, and when it doesn't crash it doesn't show my custom error message.

Reproduction

https://stackblitz.com/edit/sveltejs-kit-template-default-2dz1kb?file=src/routes/+page.server.js

Logs

[UnhandledPromiseRejection: 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(). The promise rejected with the reason "Not Found".] {
  code: 'ERR_UNHANDLED_REJECTION'
}

System Info

System:
    OS: Linux 5.19 Ubuntu 22.04.2 LTS 22.04.2 LTS (Jammy Jellyfish)
    CPU: (12) x64 AMD Ryzen 5 5600X 6-Core Processor
    Memory: 7.30 GB / 15.53 GB
    Container: Yes
    Shell: 5.8.1 - /usr/bin/zsh
  Binaries:
    Node: 18.15.0 - ~/.nvm/versions/node/v18.15.0/bin/node
    npm: 9.5.0 - ~/.nvm/versions/node/v18.15.0/bin/npm
  Browsers:
    Firefox: 112.0.1
  npmPackages:
    @sveltejs/adapter-node: ^1.2.2 => 1.2.3 
    @sveltejs/kit: ^1.15.7 => 1.15.7 
    svelte: ^3.58.0 => 3.58.0 
    vite: ^4.3.1 => 4.3.1

Severity

serious, but I can work around it

Additional Information

No response

rafaberaldo avatar Apr 27 '23 20:04 rafaberaldo

Cannot reproduce the crash with the provided repro on either stackblitz or locally, but can confirm the error reporting seems to be broken. It's logging undefined every time.

gtm-nayan avatar Apr 28 '23 01:04 gtm-nayan

It's this thing again, the default handleError hook only logs the error stack but since you're rejecting it with a string, it doesn't have one

https://github.com/sveltejs/kit/blob/526a2ed5676905807d83ace90d3853027f17f265/packages/kit/src/runtime/server/index.js#L45

Although I guess the expectation here is that you want "Custom error" to show up in the page as the error message, is that correct?

For that you'd have to reject promises with the error helper exported from @sveltejs/kit otherwise it'll get treated as an unexpected error and only send Internal Error in the response.

gtm-nayan avatar Apr 28 '23 02:04 gtm-nayan

Although I guess the expectation here is that you want "Custom error" to show up in the page as the error message, is that correct?

Yes

For that you'd have to reject promises with the error helper exported from @sveltejs/kit otherwise it'll get treated as an unexpected error and only send Internal Error in the response.

OK I got it working, the syntax should be: reject(error(404, 'Custom error'));

Thank you very much!

rafaberaldo avatar Apr 29 '23 14:04 rafaberaldo

I also seem to be encountering this issue, hard to say if it was there before because I only noticed it once I was getting fetch timeouts from the API.

Rejecting a streaming promise will crash the dev server.

Add this to page.js in the starter template:

export const load = () => ({
	stream: {
		value: Promise.reject(42)
	}
});

The terminal prints:

[UnhandledPromiseRejection: 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(). The promise rejected with the reason "42".] {
  code: 'ERR_UNHANDLED_REJECTION'
}

This is a working reproduction for me:

https://stackblitz.com/edit/sveltejs-kit-template-default-lphmtx?file=src/routes/+page.js

MarcusCemes avatar Apr 30 '23 18:04 MarcusCemes

Happens for me too

ghostdevv avatar May 05 '23 03:05 ghostdevv

Yes I have the same issue. It breaks my refresh auth logic, when I do a request inside a load function that returns a 401, I throw a redirect to /refresh/login. That works when I'm waiting inside the load or returning as a first level in the return object, but as soon as I move that request on a deeper level to stream the response, it crash the dev server

adrifer avatar May 05 '23 16:05 adrifer

@adrifer Before you tread down that path, I should let you know that you can't throw redirects from a streamed promise. The server needs to know the status code before it starts sending the response and it's too late by the time it gets to your streamed promises.

As for the issue in general, Node.js requires that an error handler must be attached before the tick in which the promise is rejected. In SvelteKit, that's done by the serializer, but by the time we're done awaiting all the load functions it may have been too late to attach the handler.

I can't think of a clean way to solve this yet, I don't think it's actually possible with the current API, we'd have to walk the returned objects (arrays, maps, whatever included) as soon as we get it from the user but turns out even that's not enough.

Consider

{ nested: { promise: Promise.reject() }, something: await setTimeout(1000) }

the rejected promise is created before the yield point of the timeout so the exception is triggered and the process exits before the promise even reaches SvelteKit.

Perhaps there is some way to kludge this, ensuring we attach the error handlers before any other yield point is reached. ~The handlers would catch the error and put it back on the promise under some special key and the serializer checks the key to get errors that happened before the promise reached the serializer~ (apparently just need to chain a catch handler, and not use the chained promise) but I won't be too hopeful that it's viable. We might need something like defer() after all.

gtm-nayan avatar May 07 '23 06:05 gtm-nayan

FWIW, I am experiencing this and it hard crashes the prod server as well. For example if I have a fetch failure it will hard crash and terminate the process:

For example if I have a fetch like this in a +page.ts and there's no server running on port 8080

export function load(ctx) {
  return {
    streamed: {
      thing: ctx.fetch("http://localhost:8080")
    }
  };
}

The production server hard crashes:

> npm run build && npm run preview
Screenshot 2023-05-12 at 5 25 33 PM

Even scarier is this happens on vercel and the server is just dead: Screenshot 2023-05-12 at 5 27 46 PM

jetaggart avatar May 13 '23 00:05 jetaggart

Here is a full repro: https://stackblitz.com/edit/sveltejs-kit-template-default-uq1e5c?file=src%2Froutes%2F%2Bpage.server.js

jetaggart avatar May 13 '23 00:05 jetaggart

This issue makes streamed data mostly unusable. Especially when dealing with fetches that may return a 404 or those that want to throw a redirect. It, for example, prevents us from using redirects to protect authentication sensitive routes.

And whats even worse is that the documentation of streamed data is suggesting that error handling isn't an issue. The example promise won't fail

            three: new Promise((fulfil) => {
                setTimeout(() => {
                    fulfil(3)
                }, 1000);
            })

and the error handling

    {#await data.streamed.three}
        Loading...
    {:then value}
        {value}
    {:catch error}
        {error.message}
    {/await}

suggests that rejected, streamed data does not crash the server.

Debugging this issue has cost us a lot of time that could have been prevent by mentioning this caveat in the documentation, or even better, implementing a fix. This should be a high priority issue, if you ask me.

MLNW avatar May 18 '23 10:05 MLNW

Agreed with @MLNW. I ran into this issue specifically with trying to throw a redirect (which is the recommended approach) if the fetch failed on an unauthorized error. Right now we'd have to wrap this in something that is guaranteed not to fail and handle explicitly in our components.

jetaggart avatar May 18 '23 17:05 jetaggart

Also the same problem, the funny thing is that the

{#await data.streamed.three}
      Loading...
  {:then value}
      {value}
  {:catch error}
      {error.message}
  {/await}

works when i navigate to this page from another route, BUT crash the server when i reload the page.

mpiorowski avatar Jun 05 '23 11:06 mpiorowski

works when i navigate to this page from another route, BUT crash the server when i reload the page.

Could it be that you have a universal instead of a server-only load function set up? Because if it is universal, the client might handle it, and the error case here happens during SSR.

knulpi avatar Jul 14 '23 15:07 knulpi

The only solution for me right now to this, while still making use of the streaming feature is to catch the error in-place and handle the object in the page. This is not the same as classical error handling with load functions, but at least I can use {#await} blocks again to show loading state to the user.

return {
	promises: {
		sectionData: GetPageData(params.pageId).then(undefined, () => {return {error: "asdf"}})
	}
}

knulpi avatar Jul 14 '23 16:07 knulpi

Still struggling with this as well. Does not seem like there's a viable solution at the moment other than to just handle the errors on the client. For redirects, I'm just check if I'm in the browser then using goto. Doesn't feel right :/

eliasdefaria avatar Jul 18 '23 03:07 eliasdefaria

this is what I am using to resolve this issue

function streamedError(err: {status: number, body: {message: string}}) { return new Promise((fulfil, reject) => { setTimeout(() => { reject(error(err.status, err.body.message)); }, 500); }); }

and then I attach it to the streamed promises as follows: streamedFunc().catch(err=>streamedError(err))

munzx avatar Aug 11 '23 18:08 munzx

Yup, this is definitely a problem. If the catch on the frontend doesn't work, this is useless. I don't think the docs need to get updated as much as this is a bug that needs fixed asap. As said above, this makes streaming unusable.

J

jdgamble555 avatar Aug 13 '23 17:08 jdgamble555

We'll probably not be able to fix this without changing a few things in a major version because your promises can reject before they even reach SvelteKit.

In the meantime you can use workarounds https://jakearchibald.com/2023/unhandled-rejections/ to prevent crashing your server. Or set up a global promise rejection handler, which would be a good idea regardless of streamed promises.

For the ones that want to throw redirects from their streamed promises, that will not work, ever, because the browser has already received an OK response with the rest of the page by that point. The best you can do there is make another navigation on the client side like @eliasdefaria said.

gtm-nayan avatar Aug 14 '23 04:08 gtm-nayan

@gtm-nayan - I think if this can't be done for now, you guys need to update the docs explaining this clearly perhaps with an example workaround. If the await catch doesn't work in the template at all, this shouldn't be the example listed.

J

jdgamble555 avatar Aug 14 '23 12:08 jdgamble555

Adding this snippet to the bottom of svelte.config.js will prevent crashes and help with debugging info during development

process.on('unhandledRejection', (reason, promise) => {
	console.error('Unhandled Rejection at: Promise', promise, 'reason:', reason);
});

oscarhermoso avatar Sep 23 '23 14:09 oscarhermoso

We'll probably not be able to fix this without changing a few things in a major version because your promises can reject before they even reach SvelteKit.

In the meantime you can use workarounds https://jakearchibald.com/2023/unhandled-rejections/ to prevent crashing your server. Or set up a global promise rejection handler, which would be a good idea regardless of streamed promises.

For the ones that want to throw redirects from their streamed promises, that will not work, ever, because the browser has already received an OK response with the rest of the page by that point. The best you can do there is make another navigation on the client side like @eliasdefaria said.

Thank you for the workaround, but I am here from my first Svelte (and JS) project, and that seems a bit complex for me.

A friend helped and I got this working. Hope it helps some newer people like me who want to use streaming promises without having the whole app crash and need a more basic workaround.

in +page.server.ts

export const load: PageServerLoad = async ({ params, locals }) => {
	const res = fetch(`http://localhost:8000/api/myendpoint`);
	return { results: { streamed: res
			.then(
				(resp) => {
					if (resp.status === 200) {
						return resp.json()
					} else if (resp.status === 404) {
						console.log("App Not found");
						return "Not Found"
					} else if (resp.status === 500) {
						console.log("API Server error");
						return "Backend Error"
					}
				}
			)
			.then(
				json=> json,
				error => {console.log('Uncaught error', error); return "Uncaught Error"}
			)
		}};
};

Then in my +page.svelte

{#await data.results.streamed}
	Loading ...
{:then results}
	{#if typeof results != 'string'}
		Loaded!
		<!-- <AppGroupCard apps={results} /> -->
	{:else}
		<p>Search failed please try again ... {results}</p>
	{/if}
{:catch error}
	<!-- NOTE: This is currently not displaying -->
	<p>Search failed please try again ... {error.message}</p>
{/await}

ddxv avatar Nov 02 '23 04:11 ddxv