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

[fix] Max call stack exceeded when large number of components are flushed #7397

Conversation

AaronDDM
Copy link

This PR is in reference to the discussion here: #7032

Without this check, flush() is being invoked multiple times, even if the previous run has not yet completed. This occurs if/when you have a large number of components. Eventually, this repeated invocation results in a max call stack exceeded error.

(I shall add a test for the original issue ASAP, get a test/pass based on the change)

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with [feat], [fix], [chore], or [docs].
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

@dummdidumm
Copy link
Member

This isn't the right fix because we would end up where we were previously to #6920 (you can see that the code prior to #6920 had exactly the check that you want to add now)

@kyrylkov
Copy link
Contributor

@dummdidumm But it would also mean it's working again, while all versions since that commit break our app.

@AaronDDM
Copy link
Author

AaronDDM commented Mar 29, 2022

This isn't the right fix because we would end up where we were previously to #6920 (you can see that the code prior to #6920 had exactly the check that you want to add now)

The while loop will actually keep going as dirty_components keeps getting added to (e.g. dirty_components.length keeps getting bigger and bigger), would it not? So this technically just prevents the same while loop being called multiple times when it is already running.

note: tests also pass with this fix, but I'll see about adding additional tests

Edit: I also tested this branch with the code from https://svelte.dev/repl/5c59fe6aa6354b03aea62c445836f2b1?version=3.44.3 which was used as a reproduction (#6760) for one of the fixes that #6920 addressed, and the fix still works, no regression.

@AaronDDM
Copy link
Author

AaronDDM commented Apr 5, 2022

@dummdidumm mind having a 2nd look at this? Happy to pivot as needed to get a resolution. Thank you!

@ruslan-khomiak
Copy link

@bluwy @Conduitry Hi guys, please tell me what needs to be done to merge this PR? Because the problem remains relevant for us and we are still on svelte version 3.44.2.

@kyrylkov
Copy link
Contributor

@dummdidumm See post above please

@dummdidumm
Copy link
Member

Please provide a reproducible (REPL or github repo) so we can investigate this properly. This PR isn't doing anything, flushing is never set to true so I can't imagine this would fix your problem. The related issue is missing a minimum reproducible.

@ruslan-khomiak
Copy link

ruslan-khomiak commented Oct 17, 2022

Please provide a reproducible (REPL or github repo) so we can investigate this properly. This PR isn't doing anything, flushing is never set to true so I can't imagine this would fix your problem. The related issue is missing a minimum reproducible.

ok, got it. I'll try to do a REPL

@AaronDDM
Copy link
Author

Hey guys, apologies I’ll take a look at updating the PR again.

@ruslan-khomiak
Copy link

ruslan-khomiak commented Oct 19, 2022

Please provide a reproducible (REPL or github repo) so we can investigate this properly. This PR isn't doing anything, flushing is never set to true so I can't imagine this would fix your problem. The related issue is missing a minimum reproducible.

@dummdidumm @Conduitry
I created a REPL and tried to simplify what I have as much as possible in order to reproduce the bug. For reproducing you need to click the "Toggle" button in the REPL.

https://svelte.dev/repl/86b8b83f2e2b475e8e92aaa4df9c7da2?version=3.44.2 - everything is good
https://svelte.dev/repl/86b8b83f2e2b475e8e92aaa4df9c7da2?version=3.44.3 - maximum call stack size exceeded error

Looks like the problem appeared after the merge of this PR: https://github.com/sveltejs/svelte/pull/6920/files

I hope this helps you find the problem and fix it.

@baseballyama
Copy link
Member

Close in favor of #8114

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants