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

Stop long running programs in client-side JS #880

Merged
merged 20 commits into from
Oct 27, 2021
Merged

Conversation

fpereiro
Copy link
Contributor

@fpereiro fpereiro commented Oct 5, 2021

How to test:

a is True
while a is True:
    b is 1
  • See that a modal appears after three seconds and that code execution is indeed interrupted.

@Felienne suggestions to the shown text are welcome! Once we agree on the text, I can add the text entry to the yamls.

@fpereiro fpereiro requested a review from Felienne October 5, 2021 13:04
@Felienne
Copy link
Member

Felienne commented Oct 6, 2021

Hi @fpereiro

Seems to work well, but we get a modal and also the regular error box:

image

Not sure if that's what I specified but just the regular error box is fine. As for the text, this is fine, if you just put this in the yaml I can make updates myself.

@Felienne
Copy link
Member

Felienne commented Oct 6, 2021

Ow and one more thing, if a program also prints, like this:

a is True
while a is True:
    print('lalala')

we first get nothing for 3 secs, then the popup and then all the output. I guess it might not be possible because it might requires threads or the like but could we get all output first and then after 3 secs the popup?

static/js/app.js Outdated
__future__: Sk.python3
__future__: Sk.python3,
// Give up after three seconds of execution, there might be an infinite loop.
execLimit: 3000
Copy link
Member

Choose a reason for hiding this comment

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

Can we put this value in a function that depends on level? For now the function can just be any level returns 3000 but that gives us the flexibility to change it depending on level.

Copy link
Member

Choose a reason for hiding this comment

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

I think you replied to this somewhere (Slack?) but I forgot the answer :) Can you repeat it here?

@fpereiro
Copy link
Contributor Author

fpereiro commented Oct 7, 2021

Hi @Felienne !

  • I just removed the modal and left the standard red error box.
  • The text is added as an error coming from the YAML instead of hardcoded in app.js.
  • The timeout is provided by an IIFE (immediately executed function) and can be customized according to level later.
  • It might be possible to print output as we go, but after digging some time through Skulpt, I'm not sure if it is possible. Skulpt itself is sparsely documented, and after digging in the implementation file for the overall family of methods we use to execute Python misceval, see here: https://github.com/skulpt/skulpt/blob/master/src/misceval.js , I don't see an obvious way to do it. If this is indeed a feature worth pursuing, I suggest we work on it as a separate issue.

@Felienne
Copy link
Member

Felienne commented Oct 7, 2021

Hi @Felienne !

  • I just removed the modal and left the standard red error box.
  • The text is added as an error coming from the YAML instead of hardcoded in app.js.
  • The timeout is provided by an IIFE (immediately executed function) and can be customized according to level later.
  • It might be possible to print output as we go, but after digging some time through Skulpt, I'm not sure if it is possible. Skulpt itself is sparsely documented, and after digging in the implementation file for the overall family of methods we use to execute Python misceval, see here: https://github.com/skulpt/skulpt/blob/master/src/misceval.js , I don't see an obvious way to do it. If this is indeed a feature worth pursuing, I suggest we work on it as a separate issue.

ok, thanks for the updates! Will try this tomorrow.

@Felienne
Copy link
Member

Felienne commented Oct 8, 2021

Hi @fpereiro

Thanks for the updates! The execution order can be a problem though, it leads to really confusing behavior.
f.e. the current implementation prohibits programs with input, f.e. this at level 7:

repeat 3 times
    dier is ask 'Welk dier kies je?'
    print 'En wie rijdt er op zijn ' dier ' door de prairie?'

After the first input, it no longer works. So maybe we need to turn the timer off for programs with ask?

Also... if you use read loud it gets interesting on your demo program, it reads once, then gives the error and then continues.

But I can imagine this all gets too complex and you want to stop working on this request? That is understandable too!!

@fpereiro
Copy link
Contributor Author

@Felienne I found a way to update the timer if there's an ask. Use the following code to test:

foo is input('bar?')
a is True
while a is True:
    b is 1

@Felienne
Copy link
Member

@Felienne I found a way to update the timer if there's an ask. Use the following code to test:

foo is input('bar?')
a is True
while a is True:
    b is 1

Nice, this is almost perfect! There is a small thing, no sure whether we can do something about it but when I enter text, the input box does not disappear until the timeout error message appears. First I thought it had not processed my input. Can that be improved (if not, we can leave it is as)

@fpereiro
Copy link
Contributor Author

HI @Felienne ! Ready to test again :). I added a timeout to ensure that the modal box is hidden before running the program.

How to test:
Go to http://localhost:5000/hedy/18
Enter this program:

foo is input('bar?')
a is True
while a is True:
    b is 1

After answering the question, the modal box should go. For three seconds, nothing should happen. Then, after three seconds, an error should be shown.

This was referenced Jan 25, 2024
@rmagedon97 rmagedon97 mentioned this pull request Mar 4, 2024
4 tasks
This was referenced Mar 8, 2024
@rix0rrr rix0rrr mentioned this pull request Mar 10, 2024
4 tasks
@rmagedon97 rmagedon97 mentioned this pull request Mar 13, 2024
4 tasks
@ArtV11 ArtV11 mentioned this pull request May 21, 2024
@bew bew mentioned this pull request Jun 4, 2024
4 tasks
mergify bot pushed a commit that referenced this pull request Jun 19, 2024
No related issue/PR, just a typo fix 🙃

**How to test**

Follow these steps to verify this PR works as intended:

* _____ (See this #880 (comment) for an example)
* _____

**Checklist**
Done? Check if you have it all in place using this list: (mark with x if done)

- [x] Contains one of the PR categories in the name
- [ ] Describes changes in the format above
- [ ] Links to an existing issue or discussion
- [ ] Has a "How to test" section

Not sure the PR template fits simple typo fixes, tell me if you need anything
@Felienne Felienne mentioned this pull request Jul 8, 2024
4 tasks
@jpelay jpelay mentioned this pull request Aug 9, 2024
4 tasks
mergify bot pushed a commit that referenced this pull request Aug 12, 2024
Please replace all _____ before you create a pull request!

Start the name of the PR with one of the relevant prefixes in the title of the PR:

* 🖊️ -> changes related to grammars or the transpiler
* 🪲 -> solving a bug
* 🚚 -> changes unrelated to shipping, such as mergify scripts
* 🧹 -> refactorings (changes that do not change functionality and are only a cleanup of code)
* 💻 -> improvements and changes of the Hedy website
* 🧪 -> changes made to the test suite
* 📜 -> changes related to content

Fixes _______ (Always link the number of the issue or of the discussion that your PR concerns. If you use the word `fixes` before the issue number in this description, the related issue will automatically close then the PR is merged)

**How to test**

Follow these steps to verify this PR works as intended:

* _____ (See this #880 (comment) for an example)
* _____

**Checklist**
Done? Check if you have it all in place using this list: (mark with x if done)

- [ ] Contains one of the PR categories in the name
- [ ] Describes changes in the format above
- [ ] Links to an existing issue or discussion
- [ ] Has a "How to test" section

If you're unsure about any of these, don't hesitate to ask. We're here to help!
@MarleenGilsing MarleenGilsing mentioned this pull request Sep 9, 2024
4 tasks
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.

while loop lvl 17 making Hedy unresponsive
2 participants