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

Avoid crashing with a StackOverflowException when iterating over the AllNodes property when it's infinitely recursive #223

Merged
merged 5 commits into from
Dec 19, 2016

Conversation

mlaily
Copy link
Contributor

@mlaily mlaily commented Nov 29, 2016

The implementations of AllNodes are delegated to an abstract internal IEnumerable<YamlNode> SafeAllNodes(RecursionLevel level) method that keeps track of the current recursion level and throws a MaximumRecursionLevelReachedException if the maximum recursion level is reached.

The maximum recursion level is currently set to 1000.

@mlaily mlaily changed the title Avoid crashing with a StackOverflowException when iterating over the AllNodes property when it's infinitely recursive [WIP] Avoid crashing with a StackOverflowException when iterating over the AllNodes property when it's infinitely recursive Nov 29, 2016
@mlaily
Copy link
Contributor Author

mlaily commented Nov 29, 2016

I also implemented the same strategy in the YamlNode.ToString() methods, but instead of throwing an exception when the node is infinitely recursive, it breaks the infinite recursion by returning an error message as a value. (See the last three commits)

@mlaily mlaily changed the title [WIP] Avoid crashing with a StackOverflowException when iterating over the AllNodes property when it's infinitely recursive Avoid crashing with a StackOverflowException when iterating over the AllNodes property when it's infinitely recursive Nov 29, 2016
@aaubry
Copy link
Owner

aaubry commented Dec 4, 2016

I'm not sure about this. Maybe AllNodes should instead return each node only once?
What is the usefulness of this property if there is no guarantee that it will work for a given document?

@mlaily
Copy link
Contributor Author

mlaily commented Dec 6, 2016

I don't know about the usefulness of AllNodes, but I'm pretty sure it's not supposed to crash the application if there is some infinite recursion :)

Returning each node only once would mean keeping a list of already returned nodes? This seems like a bad idea.

At least, with the code in this pull request, there is a catchable exception.

@aaubry aaubry merged commit 63abaa1 into aaubry:master Dec 19, 2016
@aaubry
Copy link
Owner

aaubry commented Dec 19, 2016

Sorry, I did not have time to review this earlier. I agree that it is better if AllNodes and ToString do not crash. I'll leave the discussion about the usefulness of AllNodes for later. Thanks

@mlaily
Copy link
Contributor Author

mlaily commented Dec 21, 2016

Thank you!

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.

None yet

2 participants