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

refactor(nns): Switch NNS Governance global state from static to thread_local #2844

Merged
merged 2 commits into from
Jan 2, 2025

Conversation

jasonz-dfinity
Copy link
Contributor

@jasonz-dfinity jasonz-dfinity commented Nov 26, 2024

Why governance() and governance_mut() are bad

Representing the canister global state with thread_local! avoids using unsafe blocks to access it. When using unsafe blocks to access it, one can easily write code with undefined behavior by retaining references across await boundary (more precisely, after an inter-canister call).

Why this PR

The NNS Governance heavily depends on the accessing the global state as static, and there will be a lot of refactoring needed in order to get away with the current pattern. This PR is the first step towards getting rid of those bad access patterns - with this change, we can gradually migrate from using governance()/governance_mut() to using GOVERNANCE.with(). When all the usage of governance()/governance_mut() are replaced, we can delete them and declare victory.

What

Define the global state with thread_local! (LocalKey<RefCell<Governance>>) while returning the raw pointer for the existing access pattern.

Why it is safe

The LocalKey<RefCell<T>> is set once and only once during post_upgrade or init, so the pointer should remain constant, given that the canister code is single threaded. When accessing through governance() or governance_mut() one can still write code with undefined behavior, but it is the same danger as what we have now.

Why Governance::new_uninitialized

This is needed in order for the thread_local! state to be Governance instead of Option<Governance>. We know the "uninitialized" version won't be used except for the code in the init/post_upgrade before the "initialized" state is set. However, there doesn't seem to be a good way to express that understanding. An alternative is to still use Option<Governance> and unwrap() every time, but it seems more cumbersome.

@jasonz-dfinity jasonz-dfinity requested a review from a team as a code owner November 26, 2024 23:16
@jasonz-dfinity jasonz-dfinity marked this pull request as draft November 26, 2024 23:16
@jasonz-dfinity jasonz-dfinity force-pushed the jason/experiment-thread-local branch from 448ed99 to 7c6c94c Compare November 26, 2024 23:23
rs/nns/governance/canister/canister.rs Outdated Show resolved Hide resolved
rs/nns/governance/canister/canister.rs Outdated Show resolved Hide resolved
@jasonz-dfinity jasonz-dfinity force-pushed the jason/experiment-thread-local branch 3 times, most recently from 596ab2c to 967077a Compare December 4, 2024 21:13
@jasonz-dfinity jasonz-dfinity marked this pull request as ready for review December 4, 2024 21:14
@jasonz-dfinity jasonz-dfinity force-pushed the jason/experiment-thread-local branch from 967077a to 4f5f94a Compare January 2, 2025 19:04
@jasonz-dfinity jasonz-dfinity added this pull request to the merge queue Jan 2, 2025
auto-merge was automatically disabled January 2, 2025 19:29

Pull Request is not mergeable

auto-merge was automatically disabled January 2, 2025 19:29

Pull Request is not mergeable

auto-merge was automatically disabled January 2, 2025 19:29

Pull Request is not mergeable

auto-merge was automatically disabled January 2, 2025 19:29

Pull Request is not mergeable

auto-merge was automatically disabled January 2, 2025 19:29

Pull Request is not mergeable

Merged via the queue into master with commit 76a634c Jan 2, 2025
25 checks passed
@jasonz-dfinity jasonz-dfinity deleted the jason/experiment-thread-local branch January 2, 2025 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants