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 stack overflow in FilterExpression.ValidForProperties #3946

Merged
merged 16 commits into from Aug 24, 2022
Merged

Conversation

engyebrahim
Copy link
Member

@engyebrahim engyebrahim commented Aug 19, 2022

Description

fixed the stack overflow in FilterExpression.ValidForProperties by replacing the recursion by iterative code (stacks and loops)

issue

@nohwnd
Copy link
Member

nohwnd commented Aug 19, 2022

@engyebrahim please add description to this PR, and prefix the linked issue by Fix or Fixes if you want to automatically close it. If the PR is still in progress please make it a draft.
image

@engyebrahim engyebrahim marked this pull request as draft August 19, 2022 10:31
@engyebrahim engyebrahim changed the title replaced recursion by iterative code in ValidForProperties function Fix stack overflow in FilterExpression.ValidForProperties Aug 19, 2022
@engyebrahim engyebrahim changed the title Fix stack overflow in FilterExpression.ValidForProperties Fixed stack overflow in FilterExpression.ValidForProperties Aug 19, 2022
@engyebrahim engyebrahim marked this pull request as ready for review August 22, 2022 13:29
{
valid = _condition.ValidForProperties(properties, propertyProvider);
if (!valid)
//Push root's right child and then root to stack then Set root as root's left child
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//Push root's right child and then root to stack then Set root as root's left child
// Push root's right child and then root to stack then Set root as root's left child.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that entire comment is unnecessary, and more complicated than the code :)

@nohwnd
Copy link
Member

nohwnd commented Aug 22, 2022

The filtering logic is now a bit more unclear than it was before. And even before I did not really get fully how it works. Now that you probably have understanding of what is going on, could you please add comments with explanation of how it works to the code?

Could you also add a test with a very long filter that would otherwise stack overflow please?

@nohwnd
Copy link
Member

nohwnd commented Aug 22, 2022

Even though you could theoretically measure how many iterations it would take I think it is safe to assume that 100k entries test filter should be fast to process, and enough to test that we don't stackoverflow.

(measuring how many recursive calls are possible for a simple method:

        static int MeasureStack(int count)
        {
            try
            {
                RuntimeHelpers.EnsureSufficientExecutionStack();
                return MeasureStack(++count);
            } catch (InsufficientExecutionStackException)
            {
                return count;
            }
        }

Currently this is about 15k calls for .NET for this method.
)

Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

Could you please add a test that was causing the stack overflow before so that we don't regress.

@Evangelink Evangelink enabled auto-merge (squash) August 24, 2022 08:53
@Evangelink Evangelink changed the title Fixed stack overflow in FilterExpression.ValidForProperties Fix stack overflow in FilterExpression.ValidForProperties Aug 24, 2022
@Evangelink Evangelink enabled auto-merge (squash) August 24, 2022 08:54
{
StringBuilder testCaseFilter = new("Category=Test1");

for (int i = 0; i < 1e5; i++) // creating a 100k filter cases string
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (int i = 0; i < 1e5; i++) // creating a 100k filter cases string
// Create filter with 100k conditions.
for (int i = 0; i < 100_000; i++)

Copy link
Member

Choose a reason for hiding this comment

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

just a nitpick.

@Evangelink Evangelink merged commit ad808b4 into main Aug 24, 2022
@Evangelink Evangelink deleted the pr/3905 branch August 24, 2022 10:55
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.

Test process crashes with stack overflow in FilterExpression.ValidForProperties
4 participants