3 ms·

What assertion would you have used in this case? For every comment you'd have to iterate through all it's parents to check if there is a cycle, which seems pret

What assertion would you have used in this case? For every comment you'd have to iterate through all it's parents to check if there is a cycle, which seems pretty inefficient to do for something that should never happen (there are other ways that you could check for this problem as you go, but the only other ways that I can think of require holding extra state just in order to perform the assertion). I'm for assertions when they are simple and don't cost much (especially during development), but it's not feasible to check every condition that should not happen.

petercooper 14y ago
You could assert a limit on depth, perhaps. Then the cycle would still exist but after X number of comments, the rendering ends.
- timothya 14y ago
  This is a reasonable solution. While it will (almost) never provide the correct result (it might print out a cycle of comments until X is reached, or it might cut off a very long but legitimate comment thread), it would provide a reasonable guarantee on this sort of problem not generating infinite pages.
  - petercooper 14y ago
    At the risk of being accused of flame-baiting, I'd say it's the engineering solution rather than the mathematical one.. ;-) For some reason I tend to be a fan of the "stick it in a secure box" rather than "get it right in the first place" approach..
badgar 14y ago
>iterate through all it's parents to check if there is a cycle,which seems pretty inefficient to do for something that should never happen The number of parents is almost always under 3 or 4 and never over 100. Writes occur a few times a second at peak. You are prematurely optimizing.
zzzeek 14y ago
typically, if you're operating upon a particular comment, you've gotten there by traversing to it from the parent. Ensuring that traversals don't encounter cycles is easy, keep hold of a hashtable (or a set) of comment ids as you traverse. As the traversal encounters a comment, its id is added to the hash, and as you complete traversal of each comment, the id is removed. If you encounter an id that's already in the set, assertion failed - or better yet, log the condition and then cease the traversal. That way everything keeps running and the error is visible in the logs. If the code is organized (as it should be) such that all functions which require traversal of hierarchical comments pull this from a single function, then the hash check only need be applied in that one place in the code, where it need not be visible anywhere else.

petercooper 14y ago

You could assert a limit on depth, perhaps. Then the cycle would still exist but after X number of comments, the rendering ends.

timothya 14y ago
This is a reasonable solution. While it will (almost) never provide the correct result (it might print out a cycle of comments until X is reached, or it might cut off a very long but legitimate comment thread), it would provide a reasonable guarantee on this sort of problem not generating infinite pages.
- petercooper 14y ago
  At the risk of being accused of flame-baiting, I'd say it's the engineering solution rather than the mathematical one.. ;-) For some reason I tend to be a fan of the "stick it in a secure box" rather than "get it right in the first place" approach..

badgar 14y ago

>iterate through all it's parents to check if there is a cycle,which seems pretty inefficient to do for something that should never happen The number of parents is almost always under 3 or 4 and never over 100. Writes occur a few times a second at peak. You are prematurely optimizing.

zzzeek 14y ago

typically, if you're operating upon a particular comment, you've gotten there by traversing to it from the parent. Ensuring that traversals don't encounter cycles is easy, keep hold of a hashtable (or a set) of comment ids as you traverse. As the traversal encounters a comment, its id is added to the hash, and as you complete traversal of each comment, the id is removed. If you encounter an id that's already in the set, assertion failed - or better yet, log the condition and then cease the traversal. That way everything keeps running and the error is visible in the logs. If the code is organized (as it should be) such that all functions which require traversal of hierarchical comments pull this from a single function, then the hash check only need be applied in that one place in the code, where it need not be visible anywhere else.