Use breadth first flattening order for stages (take 2) #920
Conversation
* Use breadth first flattening order for stages * Add unit tests for stage sorting
| while (stagesQueue.length > 0) { | ||
| if (Date.now() - startTime > 500) { | ||
| throw new Error("Stage sorting timed out"); | ||
| } |
There was a problem hiding this comment.
I think this is the only reliable way to make sure this loop stops after a set amount of time. I'd rather trust some other kind of global timeout but AFAIK that's not really possible in node. Let me know if that's wrong and there's some other technique we can use to set an enforceable global timeout
There was a problem hiding this comment.
nah i think this is fine! i was initially thinking "couldn't you use a settimeout for this?", but that will never resolve before the while loop is done, something something microtask queue something something event loop
There was a problem hiding this comment.
something something microtask queue something something event loop
yeah this is what I meant to say. i am curious if using worker threads and/or some kind of process/fork based server would help with this in production? is that even feasible with next? i doubt this is the last time i'll accidentally add an infinite loop (or really any cpu intensive code) and i would rather not need to have timeout logic all over the app if possible!
Issue(s) Resolved
Resolves #888 and fixes the bug that led to reverting in #917
High-level Explanation of PR
I added one new condition to the body of the loop to prevent infinite loops when the graph has a cycle. I've also added a 500ms timeout to the sorting function after which it just returns the deduped but unsorted stage list, in case there are other bugs and to make sure the app behaves reasonably with communities that have an unreasonable number of stages.