Skip to content

Conversation

@rschroll
Copy link
Contributor

I wasn't sure why the run calls were waiting for 500ms before actually running, so I took the timeouts out to see what would happen. The only think I noticed was a race condition between the exec cells and the onload macro. But the exec cells are already designed to retry if things aren't ready for them, so I just added a check that the onload has loaded as well.

Since we don't have this wait before the run function actually runs, I moved the pyodide_running flag handling inside the runPython function. Thus, this function is responsible for all the state setting; everyone else just checks it. I'm a little worried that there could be a TOCTOU bug here, if we get interrupted between the check and the function call. I don't know enough about JS concurrency to know if this is a worry or not. But it seems like a small danger, and I think it already existed in the code.

I realize I'm violating Chesterton's fence here, since I don't know that the code was supposed to do. My guesses are:

  1. Avoid the race condition between loading and exec, by delaying the exec code. If so, this is well handled in this solution.
  2. Delay the appearance of the results of the calculation. It can be hard for the user to notice the results if they appear immediately. If this were the goal, I think a better solution would be to add some animation to the appearance of the results to draw attention, while still providing them results ASAP.
  3. Something else. In this case, don't accept the PR until we figure out if it's still a problem.

As far as I can tell, this is only necessary at first load, since
the exec macro may run before on the @onload macro.  So we'll just
add an extra check that window.runPython exists before trying to
call it.

This means that we can move the setting of the pyodide_running flag
inside the runPython function, so it can be responsible for managing
that state.
@andre-dietrich
Copy link
Contributor

I will test this on a slower browser ... I think the reason for this was, that the code on slow devices started the execution while LiaScript was not informed about the operation state ... so messages from the script arrived a little bit too early ...

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