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

Infinite loop when evaluating circular references #2883

Open
CZX123 opened this issue Mar 27, 2024 · 8 comments
Open

Infinite loop when evaluating circular references #2883

CZX123 opened this issue Mar 27, 2024 · 8 comments
Labels
Bug Something isn't working critical Fixing this is mission-critical regression a feature that was working before but stopped working

Comments

@CZX123
Copy link
Contributor

CZX123 commented Mar 27, 2024

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:

@CZX123 CZX123 added Bug Something isn't working regression a feature that was working before but stopped working labels Mar 27, 2024
@RichDom2185
Copy link
Member

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
Copy link
Contributor Author

CZX123 commented Mar 29, 2024

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
Copy link
Member

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
Copy link
Member

@JoelChanZhiYang
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor Author

CZX123 commented Mar 29, 2024

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.

@CZX123 CZX123 added the important Fixing this is important, but not mission-critical label Mar 30, 2024
@martin-henz martin-henz added critical Fixing this is mission-critical and removed important Fixing this is important, but not mission-critical labels Apr 1, 2024
@martin-henz martin-henz changed the title CSE Machine infinite loop when evaluating circular references Infinite loop when evaluating circular references Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working critical Fixing this is mission-critical regression a feature that was working before but stopped working
Projects
None yet
Development

No branches or pull requests

4 participants