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 unreported endless alg splitwithlines #50279

Merged
merged 1 commit into from
Sep 24, 2022

Conversation

elpaso
Copy link
Contributor

@elpaso elpaso commented Sep 21, 2022

Fix an unreported bug I discovered while investigating #50227 (unfortunately this PR does not fix that issue).

By adding the splitted geometries to ingeoms the while loop while ( !inGeoms.empty() ) was never ending and the inGeoms list was growing until all the available memory was exhausted.

A simple test case was sufficient to reproduce the issue:

immagine

@elpaso elpaso added Processing Relating to QGIS Processing framework or individual Processing algorithms Bug Either a bug report, or a bug fix. Let's hope for the latter! Crash/Data Corruption labels Sep 21, 2022
@github-actions github-actions bot added this to the 3.28.0 milestone Sep 21, 2022
@lbartoletti
Copy link
Member

Nice! Can you add your test file please?

@elpaso
Copy link
Contributor Author

elpaso commented Sep 21, 2022

@lbartoletti
gh_50227.zip

@strk

This comment was marked as off-topic.

@nyalldawson

This comment was marked as off-topic.

@elpaso

This comment was marked as off-topic.

Copy link
Contributor

@strk strk left a comment

Choose a reason for hiding this comment

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

Lucky you, same change of mine but CI wind flowed in the right direction.
I'm approving this to help landing fixes.
Just for the record, my experience with CI and the SAME changes: https://github.com/qgis/QGIS/actions/runs/3114868919/jobs/5051179064#step:11:2995

Of course I'd love to see a testcase go with this fix, proving both the bug you reference in this PR and the bug I referenced in mine are fixed with this change. But I know it'd be very very hard.

@strk

This comment was marked as off-topic.

@m-kuhn

This comment was marked as off-topic.

@strk
Copy link
Contributor

strk commented Sep 24, 2022

@roya0045 pointed out that the C++ code was a port of a python code. It may be useful to dig the author of the initial python code to try at understanding which use case he/she had in mind with that recursion, since the testsuite didn't seem to notice the removal of it (lack of test for the usecase in mind?)

@strk

This comment was marked as off-topic.

@m-kuhn

This comment was marked as off-topic.

@strk

This comment was marked as off-topic.

@strk
Copy link
Contributor

strk commented Sep 24, 2022

same change of mine but CI wind flowed in the right direction.

For the record, I've filed this as GH-50316
ns/3114868919/jobs/5051179064#step:11:2995

@elpaso elpaso merged commit c6eca78 into qgis:master Sep 24, 2022
@elpaso elpaso deleted the bugfix-unreported-splitwithlines-endless branch September 24, 2022 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! Crash/Data Corruption Processing Relating to QGIS Processing framework or individual Processing algorithms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants