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

Catch infinite loop when passing sweep line over endpoints (#95) #97

Merged
merged 2 commits into from
Jun 29, 2020

Conversation

OliverColeman
Copy link
Contributor

This will at least allow gracefully dealing with the infinite looping, instead of chewing up heap space before ultimately crashing the entire process... Perhaps the limits could be 100,000 rather than 1,000,000 though?

@coveralls
Copy link

coveralls commented Jun 17, 2020

Pull Request Test Coverage Report for Build 319

  • 5 of 8 (62.5%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.5%) to 97.718%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/operation.js 5 8 62.5%
Totals Coverage Status
Change from base Build 316: -0.5%
Covered Lines: 701
Relevant Lines: 712

💛 - Coveralls

@mfogel
Copy link
Owner

mfogel commented Jun 18, 2020

Thanks for the PR @OliverColeman Yeah, the behavior of this lib as it runs out of heap space is frustrating as I recall - slows down and slows down and slows down and then 15 minutes later finally crashes. I assume this will help it fail earlier.

As far as test cases to trigger this behavior... do you have any coordinates that will trigger this that are of reasonable size? Like not kilobytes, not megabytes of json

Cheers

@OliverColeman
Copy link
Contributor Author

I do not have compact test cases, alas. Maybe a script could be made to randomly generate polys to try and trigger failures..?

@andrewharvey
Copy link

What I did before when I had large data which triggered a bug, was to cut the data in half, see if bug is still there, and basically repeat this until you have the absolute minimum data to replicate the issue.

@OliverColeman
Copy link
Contributor Author

@mfogel More info including small test case here: #95 (comment)
Should have posted that here, posted it on the wrong thing...

@mfogel
Copy link
Owner

mfogel commented Jun 22, 2020

So for this PR, I was thinking - could we make those max queue size and sweepLine length values configurable by environment variable? The values you have there could be used as defaults if the environment variables aren't set. Something like POLYGON_CLIPPING_MAX_QUEUE_SIZE and POLYGON_CLIPPING_MAX_SWEEPLINE_SEGMENTS. What do you think?

Oh, and we should document those env variables in the README too.

@OliverColeman
Copy link
Contributor Author

Done :)

@OliverColeman
Copy link
Contributor Author

bump This one small PR is going to make our production site so much more stable... :)

@mfogel mfogel merged commit 6df823e into mfogel:master Jun 29, 2020
@mfogel
Copy link
Owner

mfogel commented Jun 29, 2020

Thanks for the bump - Sorry I fell asleep at the wheel there 😜

I would like to let this sit on master for a few days before releasing a new version, but please give me another bump if I haven't done so by next weekend.

@mfogel
Copy link
Owner

mfogel commented Jul 3, 2020

Included in release v0.15.0, which is now live on npm.

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.

4 participants