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

Add INFINITE_LOOP_TRAP back to non-Maze app types #22463

Merged
merged 6 commits into from May 29, 2018

Conversation

joshlory
Copy link
Contributor

Some interesting history here:

I picked half a million steps sort-of-arbitrarily. It takes about 5 seconds to catch the infinite loop. I tried to balance user experience (don't freeze the browser too long) against not cutting off programs early that draw complex things.

The thrown exception is caught here:

} catch (e) {
// Infinity is thrown if we detect an infinite loop. In that case we'll
// stop further execution, animate what occurred before the infinite loop,
// and analyze success/failure based on what was drawn.

@joshlory joshlory requested a review from Hamms May 16, 2018 21:42
Copy link
Contributor

@Hamms Hamms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Circle failure looks legit.

Does this play nicely with *labs levels that we expect to run for >5e5 ticks?

@joshlory
Copy link
Contributor Author

Play Lab goes through the CustomMarshalingInterpreter.evalWithEvents path. This should only affect Artist, Craft, Eval & Calc which use CustomMarshalingInterpreter.evalWith.

@Hamms
Copy link
Contributor

Hamms commented May 17, 2018

Awesome! Have you verified the timeout against some of the longer artist drawings? And can we also remove the maze special case for this?

@joshlory
Copy link
Contributor Author

It looks like codegen is wrong for functional_definition blocks 🙄 when INFINITE_LOOP_TRAP is non-empty:

Globals.f = function(x) {
return   executionInfo.checkTimeout(); if (executionInfo.isTerminated()){return;}
  (  x +   1)
}
; return(  (  2 +   1) ==   Globals.f(  2))

@joshlory
Copy link
Contributor Author

@Hamms PTAL!

Copy link
Contributor

@Hamms Hamms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still worried about the impact this will have on the longer artist drawings, particularly the really cool fractal ones

@joshlory
Copy link
Contributor Author

Hmm — 500,000 steps is a lot. A program like that would noticeably hang most browsers. What cutoff do you think is best?

@Hamms
Copy link
Contributor

Hamms commented May 24, 2018

Oh dang, right you are. Even https://studio.code.org/projects/artist/Ut6coOwiXo_fg_B6Eh2CAA/edit only takes ~50k steps.

Worry sated! This LGTM

@joshlory joshlory merged commit 92e3084 into staging May 29, 2018
@joshlory joshlory deleted the add-back-infinite-loop-protection branch May 29, 2018 16:41
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.

None yet

2 participants