Skip to content

fix: closure circular reference fix #29

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

Closed

Conversation

wxiaoyun
Copy link
Member

@wxiaoyun wxiaoyun commented Apr 7, 2024

No description provided.

@wxiaoyun wxiaoyun added the fix label Apr 7, 2024
@wxiaoyun wxiaoyun requested a review from leonidas1712 April 7, 2024 08:39
@wxiaoyun wxiaoyun self-assigned this Apr 7, 2024
@wxiaoyun
Copy link
Member Author

wxiaoyun commented Apr 7, 2024

I think Rc::Weak may not be the solution. Consider the following code:

fn fun() {
  return () => 12;
}

let foo = fun(); // line a
// line b
foo(); // line c

At line a, fun is called and the function environment extends the block environment, lets call them env_block and env_fun.
At this moment:

  1. The runtime/thread struct has a strong reference to env_fun
  2. The Closure object of foo has a weak reference to env_fun

As fun returns (RESET instruction is ran), the runtime/thread struct drops the reference to env_fun and points to env_block. Since env_fun strong reference count is zero, env_fun is freed.

At line c, foo is called we attempt to upgrade the weak reference to env_fun and fail. The program crashes.

Untitled-2024-04-07-1650

Essentially, we need to deal with how to implement anonymous functions without it losing its environment. For now I think we need this, otherwise regular named function can create a circular reference. A potential workaround is that we let anonymous function be a special function that can hold a strong reference to its environment.

@leonidas1712
Copy link
Contributor

Yeah there could be more bugs later when trying async too. Perhaps we can keep it as it is for now then come back to this later, and if we can't fix it we can mention it in the report as a limitation.

@wxiaoyun
Copy link
Member Author

wxiaoyun commented Apr 7, 2024

@leonidas1712 ok! Just to reiterate

  • if we keep the current implementation, any named function will form a cycle with its environment and will not be freed
  • if we merge this PR, we solve the cycle problem but we cannot use function as first class values

@wxiaoyun wxiaoyun force-pushed the fix-closure-circular-reference branch from 33960f2 to 167446b Compare April 7, 2024 12:30
@wxiaoyun wxiaoyun added wontfix This will not be worked on and removed fix labels Apr 8, 2024
@wxiaoyun wxiaoyun closed this Apr 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants