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(gatsby): fix an infinite loop in node child collection #17078

Merged
merged 3 commits into from Sep 1, 2019

Conversation

@me4502
Copy link
Member

commented Aug 26, 2019

Description

Currently there are cases where a cyclic node graph can cause findChildrenRecursively to hit the limit of the stack. Also, recursive functions are notoriously slow in JS.

I've replaced this with a non-recursive implementation of a pre-order depth first search tree traversal, and also made it keep track of traversed nodes. This will prevent duplicate nodes being found, and also prevent infinite loops with cyclic node trees.

Performance wise this should theoretically be the same or possibly slightly better - I have not done a performance benchmark. Edit: Just made a change and benchmarked it, this is 3x faster than before.

Correct me if I'm wrong - but renaming this method is fine as it is not exported, and all local usages have been fixed.

@me4502 me4502 requested a review from gatsbyjs/core as a code owner Aug 26, 2019
@lannonbr lannonbr changed the title fix: fix an infinite loop in node child collection fix(gatsby): fix an infinite loop in node child collection Aug 26, 2019
me4502 added 2 commits Aug 27, 2019
…fined
@ehowey

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

Any chance this relates to what I am experiencing in this issue?

#17131

@me4502

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2019

Very unlikely as this will actually crash node due to exhausting the stack

Copy link
Member

left a comment

This is a really solid improvement! Thank you so much @me4502 💜

@gatsbybot gatsbybot merged commit 240b4f8 into gatsbyjs:master Sep 1, 2019
20 checks passed
20 checks passed
Danger All good
Details
Peril All green. Nice work.
Details
ci/circleci: bootstrap Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsby-image Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_path-prefix Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_gatsby_pipeline Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_long_term_caching Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: starters_validate Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node10 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node12 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node8 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_www Your tests passed on CircleCI!
Details
ci/circleci: windows_unit_tests Your tests passed on CircleCI!
Details
cypress: default-group 67 tests passed in 00:30
Details
unit_tests_windows Build #20190827.3 succeeded
Details
waltercruz added a commit to waltercruz/gatsby that referenced this pull request Sep 8, 2019
…17078)

* fix: fix an infinite loop in node child collection

* Flip the traversal for speed gains

* After using the cache a lot, it's possible for getNode to return undefined
@sidharthachatterjee

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

Thank you once again for your work here! Featuring it in this month's Gazette in #17548

@me4502

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2019

Nice, thanks :)

@me4502 me4502 deleted the clipchamp:fix/non-recursive-children branch Sep 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.