Scratch-vm: Can't wait longer than 2147483.647 seconds

[ericrosenbaum]

Expected Behavior

Any number of seconds entered into e.g. a "say" or "wait" block should cause the block to wait that number of seconds.

Actual Behavior

Numbers of seconds over a certain large threshold cause the wait time to be zero. We often see kids typing in very large numbers like this.

With help from @rschamp, we found the exact threshold, which is 2147483.647 (1/1000 of max int, presumably because it is converted to milliseconds). So, waiting for 2147483.647 seconds seems to work (though we didn't actually wait for 24.8 days to find out), but waiting for 2147483.648 seconds waits for 0 instead:

Steps to Reproduce

See gif

[apple502j]

In some programming languages, the max value of integers is 2147483647.
Inside the code, the block calculates 1000 times the seconds,
so 2147483.647 * 1000 = 2147483647
2147483.648 * 1000 = -2147483648

[thisandagain]

Adding 'help-wanted' to this. I would recommend modifying the math utilities to include a call to Number.isSafeInteger. If it fails, the result should be 253 - 1 (maximum double precision number as per IEEE-754).

[apple502j]

Or use unsigned int for seconds?

[Kenny2github]

@apple502j unsigned int just means the limit is higher, not gone. I would recommend an equivalent to Python 2's long integer type, if such a thing exists in JS.

[kchadha]

Sort of related, I noticed that we use two different number fields (and therefore different validation as to what characters can be typed in) for the wait (n) seconds block vs. the say/think [hello] for (n) seconds blocks. Should they be the same? The wait block does not allow typing in negative numbers while the say/think blocks do.

[thisandagain]

@kchadha Good catch! That should be fixed. The say/think and glide blocks should use the same validation as the wait block.

[Kenny2github]

@kchadha but do the number inputs in the say/think blocks have this problem as well?

[kchadha]

@Kenny2github yeah, the say/think blocks are the ones mentioned in the original bug report above.

[towerofnix]

(This is tagged with "help-wanted", not "help wanted"; probably a mistake since the vast majority of other issues are tagged "help wanted", and GitHub only recognizes issues labeled "help wanted".)

[thisandagain]

@towerofnix Thanks for catching that. The help-wanted label is a legacy from before Github added it to all repos.

[apple502j]

Ken said that setTimeout can only accept numbers less than 2147483647msec.

[mrjacobbloom]

Is it really worth it to respect wait times that high? For any wait time more than a couple days you can probably never execute the rest of the script without much fear of incompatibility. The only kind of project I can imagine actually depending on egregiously long wait times is some kind of cloud server -- or a jump scare someone plans to leave running on someone else's computer

[mzgoddard]

The blocks probably shouldn't use setTimeout. Since the outcome of the waiting only starts in a future step of the scratch engine, I think the blocks should count the frame equivalent of the amount of time they wait. Most of that is in response to this issue: https://github.com/LLK/scratch-vm/issues/904 and my comment on it https://github.com/LLK/scratch-vm/issues/904#issuecomment-362680457.

This could be done by setting a future time on the execution context (like how loops count) and yielding until that time is reached. Or setting the time to wait and decreases it by runtime.currentStepTime until its less than or equal to 0.

[AmazingMech2418]

I have an idea! Instead of setTimeout, you should use a setInterval that changes by 0.1 every 0.1 seconds or use the current timer variable for the timer block and only start the next block when the timer reaches the inputted number of seconds.

[AmazingMech2418]

I just added a PR. I don't know all of the locations with the other blocks with a setTimeout, so if you know of any, please let me know so I can update my PR.

[AmazingMech2418]

The errors are all node modules that aren't present since they can only be found in gh-pages and not develop.

[ericrosenbaum]

Thanks for your thoughts on this folks! The issue here is the integer overflow problem, so any PR addressing this issue should be a fix for that. We should go with the fix recommended by @thisandagain above.

Sorry @amazinigmech2418, we will need to close your PR. It seems to be addressing the one @mzgoddard mentioned, which is actually documented in https://github.com/LLK/scratch-vm/issues/904, but is not the fix for that issue which we want. For that one, we should be using the VM's stack timer and yield mechanisms (as I did for the timing in the music extension, e.g. here).

[joker314]

Note that the numbers for which Scratch breaks are actually safe integers (i.e., Number.isSafeInteger(2147483648) is true.

This is because that number is 2 ** 31, but Number.MAX_SAFE_INTEGER is 2 ** 53 - 1.

This is a problem that should only affect setTimeout, because it apparently uses a signed 32-bit integer internally.

We could create something as a maths utility to convert numbers to signed 32-bit numbers (e.g. Math.max(n, 2 ** 31 - 1). We could also create a custom setTimeout function (e.g. setSafeTimeout). I prefer the former.

[ericrosenbaum]

@thisandagain discussed this and we decided to replace setTimeout in each case with a system that uses the stack timer and yielding. We'll use Number.isSafeInteger to prevent the specific problem described in this issue.