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

Terminate loops after 1s timeout or 10,000 iterations #9

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wylieconlon
Copy link
Contributor

This is a fix for popcodeorg/popcode#1354, "Loop breaking doesn't work if there's a console.log() in the loop body"

@wylieconlon
Copy link
Contributor Author

I updated the code to sample every 100 loops

@wylieconlon
Copy link
Contributor Author

@outoftime Looking for a review on this

@outoftime
Copy link
Contributor

@wylieconlon so I don’t have any particular objection to this, but it doesn’t appear to solve the original issue—a loop with a lot of console.log statements still locks up the environment. Doesn’t have to be an infinite loop at all; a simple count up to 10,000 does the trick quite nicely.

I suspect this has to do with the way the JavaScript execution is scheduled between Popcode itself (window.top) and the preview frame. Each time console.log() is called, it’s sending a postMessage up to window.top which causes an excessively expensive Redux update (particularly when performed in a tight loop). Once the top-level execution frame starts chugging, that takes priority over the execution of the loop (and the loop breaker) in the iframe. A couple years ago I did some experimentation and found evidence that browsers (at least Chrome) prioritize JavaScript execution in the top-level window over execution in iframes (which also makes intuitive sense).

Anyway, I pushed this to version 0.1.1-alpha.1 on npm, so feel free to install it in your local Popcode environment and mess around with it. LMK if you think it’s still worth merging!

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.

2 participants