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

Process: remove circular weak reference #2872

Merged
merged 7 commits into from
Apr 17, 2023

Conversation

sporksmith
Copy link
Contributor

@sporksmith sporksmith commented Apr 13, 2023

This unblocks some further cleanup of managing Process's internal state, and removes some complexity in itself.

@sporksmith sporksmith self-assigned this Apr 13, 2023
@github-actions github-actions bot added the Component: Main Composing the core Shadow executable label Apr 13, 2023
@sporksmith sporksmith force-pushed the remove-process-circ-ref branch 3 times, most recently from 125a1ef to 0dd8a77 Compare April 17, 2023 18:50
src/main/host/thread.rs Outdated Show resolved Hide resolved
Passing the outer ProcessRefCell wrapper to C code was primarily to help
with an intermediate state where some of Process was still in C. It also
allowed us to potentially catch more conflicting borrow errors at runtime,
but these shouldn't be an issue in practice since the C code no longer
stores these pointers.
Specifically this will let us change the Worker::set_active_process API
to take a RootedRefCell<RefCell<Process>>, without needing a circular
reference in Process.

Later this will also be a good place to handle some of Process teardown
when the Process exits.
This removes usage of the circular weak reference inside Process.
This makes the interface consistent with Worker::set_active_process, and
simplifies the implementation.
@sporksmith sporksmith merged commit 3a076f0 into shadow:main Apr 17, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Main Composing the core Shadow executable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants