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

Prevent infinite loop while splitting pages #2822

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mantljosh
Copy link

The existing check for whether all children got moved the next page makes sense if we're already partly through the page - but if this happens before we have anything in currentChildren then we'll essentially leave a blank page and move everything to the "next" page, which results in an infinite loop (since the next page never gets smaller).

This PR checks for this situation, and in this case will add the current node to the current page and push the remaining nodes to the next page. This ensures that the next page will get smaller by at least one node so that future iteration won't infinitely loop.

I added a test case that reproduces it - the test doesn't fail it will just hang forever, but if it doesn't loop then the test will pass successfully.

Fixes #2659

Copy link

changeset-bot bot commented Jul 18, 2024

⚠️ No Changeset found

Latest commit: ec55b62

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@carlobeltrame carlobeltrame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change seems correct to me (who wrote the current splitting algorithm), thanks for fixing this! I don't have time to test it manually, unfortunately.

@diegomura
Copy link
Owner

Thanks for this. Unfortunately I have very little time lately and can't test right now. Can someone validate it? 🙏🏻

@mantljosh
Copy link
Author

mantljosh commented Aug 21, 2024

FWIW I have tested this in my real world scenario (by linking to a locally built copy of react-pdf) that I was running into and it appears to be working properly. @jakeprem did a bunch of investigation in #2659 (huge kudos to him for this btw) - perhaps he'd be able to validate as well.

@mantljosh
Copy link
Author

@diegomura can this be merged/released?

@ArturCzopek
Copy link

Would be awesome to release because this is probably the reason why our scenario does not work as well and page is crashing all the time

@ArturCzopek
Copy link

@mantljosh could you help install your version locally and make it as a permanent dependency? I am not strict front-end dev but I hope that your fix could solve lots of issues we have

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.

Infinite loop hanging megathread (minPresenceAhead, margin, break)
4 participants