Skip to content

[🐞] PrefetchServiceWorker throws insufficient resources when navigating while prefetching #6602

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

Closed
DustinJSilk opened this issue Jun 25, 2024 · 15 comments
Assignees
Labels
STATUS-2: team is working on this Scheduled for work by the core team TYPE: bug Something isn't working

Comments

@DustinJSilk
Copy link
Contributor

Which component is affected?

Qwik Runtime

Describe the bug

While the PrefetchServiceWorker prefetches the graph, if you interact/navigate to a new page which triggers another prefetch, the browsers throws errors with insufficient resources.

Please see the below video.

Screen.Recording.2024-06-25.at.12.07.58.mov

Reproduction

https://github.com/DustinJSilk/qwik-prefetch-nav-crash

Steps to reproduce

$ ./generate.sh

$ pnpm i

$ pnpm preview

Click on the page link before the graph finishes prefetching and see 404s thrown.

System Info

n/a

Additional Information

No response

@DustinJSilk DustinJSilk added STATUS-1: needs triage New issue which needs to be triaged TYPE: bug Something isn't working labels Jun 25, 2024
@gioboa
Copy link
Member

gioboa commented Jun 25, 2024

How can we solve this issue? @DustinJSilk do you have a possible solution?

@DustinJSilk
Copy link
Contributor Author

I'm not too clued up on the current implementation, but would this not point to an issue where $maxPrefetchRequests$ isn't being respected, which means the dependencies for the linked page are all being fetched with the highest priority?
So the issue here would actually be how the graph is built where all 750 child pages are set as required dependencies for the navigated page to load when they aren't needed yet

@gioboa gioboa linked a pull request Jun 25, 2024 that will close this issue
7 tasks
@gioboa
Copy link
Member

gioboa commented Jun 25, 2024

Can you test this version in your environment pls? It works fine for me.

@DustinJSilk
Copy link
Contributor Author

I'm still getting the same issue mentioned above.

It seems to exist in reverse order as well in a slightly different way, maybe this will help to identify the problem:
When first loading the child page, it doesn't prefetch all the symbols yet. Then, when navigating to the home page it prefetches all the symbols in parallel causing the same error as above. I've attached a video below showing this similar issue.

Screen.Recording.2024-06-26.at.11.17.28.mov

@DustinJSilk
Copy link
Contributor Author

As I think about it, i wonder if its safe to allow the browser to throw these errors since as long as they eventually recover. We don't know what the limit is per device and don't want to restrict how fast we prefetch if the files are high priority.

So I think the real issue here isn't the fact that the errors are being thrown, they can be ignored, but rather that we're fetching bundles with high priority when they aren't in fact high priority.

@gioboa
Copy link
Member

gioboa commented Jun 27, 2024

In my PR the dependencies have less priority than the "prefetch" bundles. I think we need to limit the requests in parallel

@gioboa
Copy link
Member

gioboa commented Jul 9, 2024

It's similar to #6150 because here is fetching all the pages basically

@gioboa gioboa self-assigned this Aug 6, 2024
@gioboa gioboa added the STATUS-2: team is working on this Scheduled for work by the core team label Aug 6, 2024
@gioboa gioboa removed the STATUS-1: needs triage New issue which needs to be triaged label Aug 6, 2024
@jwickers
Copy link
Contributor

I was considering opening another issue but this might be related.

In commit 548c7d0 there was a change in how prefetch would trigger which caused this bug (or a very similar one) for us when updating.
Prior to that the onVisible prefetch would only trigger on "mobile" devices (windowInnerWidth < 520), after it would also trigger on "desktop".

Now in my instance we had a dropdown navigation menu that would expand on hover, meaning dragging the mouse over the menu caused all the links to become visible and start prefetching at once, potentially queuing way too many prefetch, and clicking on a link would then be ignored until the prefetch were done.

So had to disable prefetch on the menu for now, but it would be nice to have an option to disable prefetch onVisible in some cases.
Alternatively if there is supposed to be a fix in this issue where we either have a max prefetch that would not delay the onClick navigation from occurring, or adding a cancel signal to the prefetch (abort controller?) to immediately stop prefetching if there is a navigation event happening, this would be nice.

@jwickers
Copy link
Contributor

BTW the doc here: https://qwik.dev/docs/routing/#link-prefetch does not mention the onVisible prefetching

@DustinJSilk
Copy link
Contributor Author

I would say disabling the prefetch would be the wrong solution. What should happen is that the requests for the link you’re navigating to should be fetched with the highest priority so that it doesn’t need to wait for the rest of the prefetches to complete.

@thejackshelton
Copy link
Member

thejackshelton commented Aug 19, 2024

I think what's going on here is the moment a possible interaction is on the page, the new prefetch service worker will start grabbing anything it can in the application and start adding it to the bundle graph, regardless of if there is any possible links to navigate to on the page.

prefetch-issue-low.mp4

This is a more minimal reproduction, following the same reproduction steps. We create a bunch of routes, and then there is nothing on the page except a click handler and a QRL
https://github.com/thejackshelton/prefetching-world-repro

It seems to be a bug in the new service worker implementation. Above I am using incognito mode, to see it do the buffering.

I'll look more into the implementation 🫡

@wmertens
Copy link
Member

wmertens commented Sep 8, 2024

confirmed fixed with #6853

@DustinJSilk
Copy link
Contributor Author

Amazing, looking forward to getting these changes to production soon. Thanks @wmertens

@shairez
Copy link
Contributor

shairez commented Sep 17, 2024

@DustinJSilk now that 1.9 is out, can you please check if it solved the issue?

Thanks!

@DustinJSilk
Copy link
Contributor Author

This looks to be solved now with the reproduction repo.
I'll get this added to my app this week to see how it performs.
Thanks for getting this done, everyone!

@github-project-automation github-project-automation bot moved this from Waiting For Review to Done in Qwik Development Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
STATUS-2: team is working on this Scheduled for work by the core team TYPE: bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants