Added loop guard to loops that have a dynamic exit condition.

[erikh2000]

Please see #63 for the rationale behind this.

There were just two loops in the code that have a dynamic exit condition. I didn't see any reason to guard the other loops, because they have a constant exit condition, so their behavior is easily predictable. Of the two places where a guard is added in this PR, only the inner loop of connectEdges() has generated infinite loops that I'm aware of. There could be some fundamental change made to the loop code where we are 100% sure it will never inf-loop, but this guard will keep browsers from crashing until that time. Have tested this successfully on the known cases that I reported.

[rowanwins]

Just one question on this one @erikh2000 - can you run the benchmark test and post the results with and without the loop guard just so we can be sure it's not too detrimental. Thanks

[erikh2000]

Sure. I ran bench a few times with this PR in and a few times with this PR out. Overall time seems to change by +/- 2 seconds for me from run to run even with no changes to code.

Overall time (with loop guard):

• run 1: 36.90 secs
• run 2: 36.23 secs

Overall time (no loop guard):

• run 1: 35.71s
• run 2: 37.77s

An example run with this PR (loop guard) in:

tcc-EHermansen:martinez erik.hermansen$ yarn run build
yarn run v0.24.5
$ browserify -s martinez ./index.js -o ./dist/martinez.js 
✨  Done in 0.60s.
tcc-EHermansen:martinez erik.hermansen$ yarn run bench
yarn run v0.24.5
$ node bench.js 
Hole_Hole
Martinez x 9,368 ops/sec ±4.54% (83 runs sampled)
JSTS x 1,344 ops/sec ±7.59% (82 runs sampled)
- Fastest is Martinez

Asia union
Martinez x 3.86 ops/sec ±8.83% (14 runs sampled)
JSTS x 5.37 ops/sec ±5.65% (18 runs sampled)
- Fastest is JSTS

States clip
Martinez x 95.68 ops/sec ±1.14% (69 runs sampled)
JSTS x 72.55 ops/sec ±1.29% (66 runs sampled)
- Fastest is Martinez

✨  Done in 36.90s.

[gap777]

@rowanwins can we get this fix for the infinite loop merged? We're running into some infinite loop scenarios too.

[HansBrende]

@w8r I am also running into infinite loops but no way to debug them, because javascript. Cannot even debug the infinite loops or other errors until this PR (or similar one) is merged, because I am just hanging, no way to know when to log as it is too late to do so once the infinite loop starts... No way to cancel, timeout, force quit, nothing. Probably I will just look for a different library at this point, infinite loops are a dealbreaker.

[erikh2000]

I think it makes sense to add the loop guard even though it may hide errors. The inf. loop will crash browser pages which is quite severe. But that's not my decision. You might also file a bug with the specific call params that generate the infinite loop.

…

On Thu, Aug 5, 2021, 12:02 PM Hans Brende ***@***.***> wrote: @w8r <https://github.com/w8r> I am also running into infinite loops but no way to debug them, because javascript. Cannot even debug the infinite loops or other errors until this PR (or similar one) is merged, because I am just hanging, no way to know when to log as it is too late to do so once the infinite loop starts... probably I will just look for a different library at this point, infinite loops are a dealbreaker. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#66 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACB3GHQMJW5WUTC6YBE6LUDT3LN4HANCNFSM4EQLKV5A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .