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.

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

Defer not working properly when multiple promises are returned #9154

Open
paoloricciuti opened this issue Feb 21, 2023 · 27 comments
Open

Defer not working properly when multiple promises are returned #9154

paoloricciuti opened this issue Feb 21, 2023 · 27 comments
Labels
pkg:adapter-cloudflare pkg:adapter-vercel Pertaining to the Vercel adapter

Comments

@paoloricciuti
Copy link
Member

Describe the bug

I've tryed the new defer api with vercel edge functions and when a single promise is returned it works as intended. However when you return multiple promises things get's messy quickly.

For example

const wait = (ms) => new Promise((r) => setTimeout(r, ms));

async function getBlog() {
	await wait(2000);
	return 'A cool blog post';
}

async function getComments() {
	await wait(4000);
	return ['Very cool', 'Super', 'Fantastic'];
}

async function getRecommended() {
	await wait(1000);
	return ['Another post', 'Suh interesting'];
}

async function getFail() {
	await wait(8000);
	throw new Error('Dang!');
}

export async function load() {
	const recommended = getRecommended();
	const comments = getComments();
	const fail = getFail();
	const blog = await getBlog();
	return {
		blog,
		defer: {
			recommended,
			comments,
			fail
		}
	};
}

this configuration correcly wait 2 seconds before showing the page. The recommended shows a sliver of loading state to than resolve immediately however for getComments and getFail the loading state perpetrate until both of them resolve at the same time. When i was trying this at the beginning i saw the same kind of result with only getRecommended + getComments (in this case getRecommended that should already be resolved was persisting in the loading state for a couple of seconds).

2023-02-21.16-42-29.mp4

Reproduction

You can see the live version of this on vercel.

You can see the code on my github repo and edit the code on stackblitz

Logs

No logs

System Info

System:
  OS: Linux 5.0 undefined
  CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
  Memory: 0 Bytes / 0 Bytes
  Shell: 1.0 - /bin/jsh
Binaries:
  Node: 16.14.2 - /usr/local/bin/node
  Yarn: 1.22.19 - /usr/local/bin/yarn
  npm: 7.17.0 - /usr/local/bin/npm
npmPackages:
  @sveltejs/adapter-auto: ^2.0.0 => 2.0.0 
  @sveltejs/adapter-vercel: ^2.0.4 => 2.0.4 
  @sveltejs/kit: ^1.5.0 => 1.8.3 
  svelte: ^3.54.0 => 3.55.1 
  vite: ^4.0.0 => 4.1.3

Severity

blocking an upgrade

Additional Information

No response

@dummdidumm
Copy link
Member

We just released a change to the vercel adapter to improve streaming there, could you check again with the latest version of the adapter?

@paoloricciuti
Copy link
Member Author

paoloricciuti commented Feb 21, 2023

We just released a change to the vercel adapter to improve streaming there, could you check again with the latest version of the adapter?

Just upgraded to @sveltejs/adapter-vercel 2.1.0 but the issue is still there

You can also check stackblitz+the repo+the demo to verify yourself

@Smirow
Copy link

Smirow commented Feb 21, 2023

if you surround your page with a {#key data} ... {/key} it seems to work as expected. Maybe more an issue with Svelte it self.
edit: Actually it does not work as expected, but has another behavior.

@paoloricciuti
Copy link
Member Author

if you surround your page with a {#key data} ... {/key} it seems to work as expected. Maybe more an issue with Svelte it self. edit: Actually it does not work as expected, but has another behavior.

If I update surround everything with {#key data} it still doesn't work because it loads the recommendation (that should already be resolved) together with the comments. It does however reject the failing promise at a later time

@Smirow
Copy link

Smirow commented Feb 21, 2023

Yep. Seems to work fine on adapter-node, maybe more of a Vercel thing (on both edge and serverless functions).

@Smirow
Copy link

Smirow commented Feb 21, 2023

Seems it's out of sync: the first promise resolve when the second is actually resolved, the second resolve when the third resolve so on and so forth until last two promises which are resolved at the same time.

@Smirow
Copy link

Smirow commented Feb 22, 2023

Ooh, to add to that, this doesn't occur on client-side navigation where it works just fine.

@dummdidumm
Copy link
Member

My guess at this point is that this has to do with some browser or server behavior where things are buffered and multiple resolved promise updates come in at once

@paoloricciuti
Copy link
Member Author

Could this be a vercel-adapter behavior?

@Smirow
Copy link

Smirow commented Feb 22, 2023

Digging around, it seems that it's the brotli compression used by default on Vercel that is causing this issue. If I force gzip on the request header Accept-Encoding: gzip it works as expected.

@paoloricciuti
Copy link
Member Author

I've tried to use setHeaders on the load function without success...however inspecting the network i see this

image

on the response headers.

I also see content-encoding: br.

This might be worth another issue given the setHeaders seem to be bugged too when using streaming. @dummdidumm let me know if have to open another issue.

P.s. @Smirow How to force the accept-encoding header on the request? By using the server hook?

@Smirow
Copy link

Smirow commented Feb 22, 2023

@paoloricciuti it's a request header, you'll need it to set it on your browser, on chrome you can use ModHeader for instance. It doesn't solve the issue though, as you can't force Vercel to use a specific compression algorithm by default and will choose brotli or gzip based on what is supported by the client (i.e. the Accept-Encoding header).

For the response header, you'll need to pass it a record not a string: setHeaders: (headers: Record<string, string>)

@paoloricciuti
Copy link
Member Author

paoloricciuti commented Feb 22, 2023

@paoloricciuti it's a request header, you'll need it to set it on your browser, on chrome you can use ModHeader for instance. It doesn't solve the issue though, as you can't force Vercel to use a specific compression algorithm by default and will choose brotoli or gzip based on what is supported by the client (i.e. the Accept-Encoding header).

Yeah i realized after trying that you were setting a request header that's why i asked if there was a way to force it via code later on.

For the response header, you'll need to pass it a record not a string: setHeaders: (headers: Record<string, string>)
I always seem to forget this lol

@Smirow
Copy link

Smirow commented Feb 22, 2023

Ooh, to add to that, this doesn't occur on client-side navigation where it works just fine.

That's maybe because the page size is too small and don't get compressed, will have to try with a larger page.

Maybe Simon or Rich can get it touch with the competent people at Vercel?

@paoloricciuti
Copy link
Member Author

Ooh, to add to that, this doesn't occur on client-side navigation where it works just fine.

That's maybe because the page size is too small and don't get compressed, will have to try with a larger page.

Maybe Simon or Rich can get it touch with the competent people at Vercel?

I think it's because maybe with client side navigation it doesn't stream the entire page but just the data?

@Smirow
Copy link

Smirow commented Feb 22, 2023

Oh actually during client side navigation the response is sent with content-type: text/sveltekit-data so it will never be compressed.

@paoloricciuti
Copy link
Member Author

Hm i've tried to deploy the same app with remix and it seems to work fine.

I don't have a stackblitz but you can see the code on github and the live demo at https://test-defer-remix.vercel.app/

So maybe it's not the brotli thing

@paoloricciuti
Copy link
Member Author

Another intresting difference is that remix html comes already with the data if the promise is already resolved (for example the recommended post if you see the rendered html does not have the loading state)

@Smirow
Copy link

Smirow commented Feb 22, 2023

Yes definitely bizarre, some buffering is going on, can't manage to understand why.

@Smirow
Copy link

Smirow commented Feb 24, 2023

The chunks get truncated sometimes :

test.mov

(the end </script> tag is probably just Chrome doing its things)

And also seems that \n is not enough to always trigger script evaluation, \r\n seems more reliable:

push(`<script>${global}.resolve(${str})</script>\n`);

And to add to that, the behavior described above by @paoloricciuti is a bit flaky depending on the build, sometimes the build just work fine (neglecting the script evaluation), sometimes it doesn't.

@dummdidumm
Copy link
Member

Another intresting difference is that remix html comes already with the data if the promise is already resolved (for example the recommended post if you see the rendered html does not have the loading state)

This is a limitation in Svelte core, which we plan to address later

@mikerowe81
Copy link

I ran into similar issues with adapter-node I believe just in dev build. It seemed to randomly work and randomly not work between SSR and CSR. Didn't spend a lot of time troubleshooting because while the feature sounds great it wasn't critical for my current needs so I just stopped using it hoping it will be fixed later. If I can help troubleshoot something specifically, let me know.

@Smirow
Copy link

Smirow commented Mar 5, 2023

It's not supposed to work with csr = false if it's what you're doing.

@dummdidumm dummdidumm added the pkg:adapter-vercel Pertaining to the Vercel adapter label Mar 6, 2023
@Rich-Harris
Copy link
Member

I don't have a fix for this issue yet but didn't want to leave it uncommented. As far as I can tell browsers batch up content below a certain number of bytes, and when the response is compressed the chunks come in under that threshold. Adding a bunch of junk data (e.g. <!-- ${random_bytes(1024)} -->) seems to 'fix' it but is obviously a suboptimal solution.

@mikerowe81
Copy link

To clarify, I was not using csr = false I seemed to have random issues when navigating to the page from another page in the app (CSR) and when reloading the page directly (SSR)

@paoloricciuti
Copy link
Member Author

I don't have a fix for this issue yet but didn't want to leave it uncommented. As far as I can tell browsers batch up content below a certain number of bytes, and when the response is compressed the chunks come in under that threshold. Adding a bunch of junk data (e.g. <!-- ${random_bytes(1024)} -->) seems to 'fix' it but is obviously a suboptimal solution.

Thanks for the comment, just to clarify, the junk data should be inside the actual promise? From your comment seems like it can go inside the await but to me it doesn't make sense

@markjaquith
Copy link
Contributor

This is labeled as pkg:adapter-vercel but I suspect this is also happening with Cloudflare.

I've noticed that specifically when using Chrome on Cloudflare and returning multiple streaming promises, sometimes just nothing returns. If I use Firefox, it works. If I'm on the local dev server with Firefox or Chrome, it works.

I've switched to using a single streaming promise for the main data that I care about (which unfortunately means I have to do a proxied fetch for the "supplementary" data), and now it is reliably working on Cloudflare/Chrome.

If this issue gets resolved for Vercel and still remains for Cloudflare, I'll open a ticket. But it feels like it could be related.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:adapter-cloudflare pkg:adapter-vercel Pertaining to the Vercel adapter
Projects
None yet
Development

No branches or pull requests

6 participants