source-academy / frontend

Frontend of Source Academy, an online experiential environment for computational thinking (React, Redux, Saga, Blueprint)
https://sourceacademy.org
Apache License 2.0
103 stars 168 forks source link

Infinite loop when evaluating circular references #2883

Open CZX123 opened 6 months ago

CZX123 commented 6 months ago

A circular reference inside the playground code leads to an infinite loop, crashing the Source Academy. An example can be seen by running the below code:

const a = [];
a[0] = a;

This affects all available runners of all TS, JS, and Scheme variants, except Source §3 Concurrent (which is based on a VM).

The console error appears to be caused by "immer" module, but the error is logged from SafeEffects.ts:

RichDom2185 commented 6 months ago

The console error appears to be caused by "immer" module

Can you elaborate? I can't immediately see why Immer would be causing the error.

CZX123 commented 6 months ago

Now it's giving me a slightly different error:

It seems like Immer is still handling the context objects like environment trees when the CSE Machine gets run, which results in this error as there are definitely circular references inside.

RichDom2185 commented 6 months ago

It seems like Immer is still handling the context objects like environment trees when the CSE Machine gets run, which results in this error as there are definitely circular references inside.

I see, the problem is with the design of the app: the js-slang context should have never been part of the frontend state in the first place since the golden rule of redux is to always ensure that state is immutable. The fact that js-slang context is mutable makes us resort to a lot of stuff like disabling auto-freeze and other workarounds.

Hmm, we really need to refactor code evaluation soon….

@sayomaki @JoelChanZhiYang sorry to ping you out of nowhere but do you have any thoughts?

RichDom2185 commented 6 months ago

See also: https://immerjs.github.io/immer/pitfalls/

JoelChanZhiYang commented 6 months ago

Just to clarify, is the circular reference inside the js-slang context intended behavior?

Or in other words, is the root cause of the bug the fact that js-slang has a circular reference? Or is it because immer does not allow of circular references?

RichDom2185 commented 6 months ago

Just to clarify, is the circular reference inside the js-slang context intended behavior?

Yes.

Or in other words, is the root cause of the bug the fact that js-slang has a circular reference? Or is it because immer does not allow of circular references?

I'd say both are intended behavior, i.e. the root cause is actually the fact that we have the execution context (which is mutable) being put in redux (which expects everything to be immutable), thereby violating Redux's golden rule, causing the incompatiblity when we move to modern redux (aka RTK, with Immer).

Immer was just doing its job.

JoelChanZhiYang commented 6 months ago

Ah I see. The context seems to be used all over the place. But it seems like it is mostly being consumed rather than modified. It is modified in UPDATE_SUBLANGUAGE and I assume when the various eval functions are called.

How do you foresee the refactor to look like? Will the entire js-slang context still be stored? If so, where will it be stored?

CZX123 commented 6 months ago

Actually, the error only occurs when the code that is written in the playground editor has the circular reference, it seems like the rest of the js-slang context is still fine despite also having circular references, maybe that could give a clue?

Edit: seems like source 3 non-det also does not work, and will crash regardless of what is written in the editor.