-
Notifications
You must be signed in to change notification settings - Fork 12
fix(bindings): refcount Pool to fix teardown panic #352
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
base: main
Are you sure you want to change the base?
Changes from all commits
e2c6733
53db9cd
393a0e3
4a19a35
f7366ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,6 +66,7 @@ pub const js_meta = js.class(.{ .properties = .{ | |
| } }); | ||
|
|
||
| cached_state: ?*CachedBeaconState = null, | ||
| pool_rc: ?*pool.PoolRc = null, | ||
| const BeaconStateView = @This(); | ||
|
|
||
| pub fn init() BeaconStateView { | ||
|
|
@@ -78,6 +79,10 @@ pub fn deinit(self: *BeaconStateView) void { | |
| allocator.destroy(cached_state); | ||
| self.cached_state = null; | ||
| } | ||
| if (self.pool_rc) |rc| { | ||
| rc.unref(); | ||
| self.pool_rc = null; | ||
| } | ||
| } | ||
|
|
||
| // ------------------------- | ||
|
|
@@ -90,7 +95,7 @@ pub fn createFromBytes(bytes: js.Uint8Array) !BeaconStateView { | |
| const byte_slice = try bytes.toSlice(); | ||
| const slot_value = fork_types.readSlotFromAnyBeaconStateBytes(byte_slice); | ||
| const fork_seq = config.state.config.forkSeq(slot_value); | ||
| state.* = try AnyBeaconState.deserialize(allocator, &pool.state.pool, fork_seq, byte_slice); | ||
| state.* = try AnyBeaconState.deserialize(allocator, pool.state.pool(), fork_seq, byte_slice); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| errdefer state.deinit(); | ||
|
|
||
| const cached_state = try allocator.create(CachedBeaconState); | ||
|
|
@@ -107,7 +112,10 @@ pub fn createFromBytes(bytes: js.Uint8Array) !BeaconStateView { | |
| null, | ||
| ); | ||
|
|
||
| return .{ .cached_state = cached_state }; | ||
| return .{ | ||
| .cached_state = cached_state, | ||
| .pool_rc = pool.state.poolRc().ref(), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| }; | ||
| } | ||
|
|
||
| // ------------------------- | ||
|
|
@@ -781,7 +789,7 @@ pub fn createMultiProof(self: *const BeaconStateView, descriptor: js.Uint8Array) | |
|
|
||
| var proof = persistent_merkle_tree.proof.createProof( | ||
| allocator, | ||
| &pool.state.pool, | ||
| pool.state.pool(), | ||
| root_node, | ||
| proof_input, | ||
| ) catch { | ||
|
|
@@ -934,7 +942,10 @@ pub fn processSlots(self: *const BeaconStateView, slot_arg: js.Number, options: | |
| } | ||
|
|
||
| try st.processSlots(allocator, napi_io.get(), post_state, slot_value, .{}); | ||
| return .{ .cached_state = post_state }; | ||
| return .{ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| .cached_state = post_state, | ||
| .pool_rc = pool.state.poolRc().ref(), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| }; | ||
| } | ||
|
|
||
| /// Compute expected withdrawals for the next payload (capella+). | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,42 +1,61 @@ | ||
| const std = @import("std"); | ||
| const js = @import("zapi:zapi").js; | ||
| const Node = @import("persistent_merkle_tree").Node; | ||
| const RefCount = @import("state_transition").RefCount; | ||
|
|
||
| /// Pool uses page allocator for internal allocations. | ||
| /// It's recommended to never reallocate the pool after initialization. | ||
| const allocator = std.heap.page_allocator; | ||
|
|
||
| const default_pool_size: u32 = 0; | ||
|
|
||
| pub const PoolRc = RefCount(Node.Pool); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the style guide (Rule 224), acronyms should use proper capitalization. Since References
|
||
|
|
||
| /// Pool is wrapped in `RefCount` so binding objects holding pool refs at | ||
| /// process exit keep the pool alive until their JS finalizer runs. NAPI | ||
| /// env cleanup hook fires before module-level JS holders are finalized, | ||
| /// so an unconditional `pool.deinit()` there would free memory that | ||
| /// `pool.unref()` calls in those finalizers still need. | ||
| pub const State = struct { | ||
| pool: Node.Pool = undefined, | ||
| initialized: bool = false, | ||
| pool_rc: ?*PoolRc = null, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| pub fn init(self: *State) !void { | ||
| if (self.initialized) return; | ||
| self.pool = try Node.Pool.init(allocator, default_pool_size); | ||
| self.initialized = true; | ||
| if (self.pool_rc != null) return; | ||
| var pool_value = try Node.Pool.init(allocator, default_pool_size); | ||
| errdefer pool_value.deinit(); | ||
| self.pool_rc = try PoolRc.init(allocator, pool_value); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| } | ||
|
|
||
| pub fn deinit(self: *State) void { | ||
| if (!self.initialized) return; | ||
| self.pool.deinit(); | ||
| self.initialized = false; | ||
| if (self.pool_rc) |rc| { | ||
| rc.unref(); | ||
| self.pool_rc = null; | ||
| } | ||
| } | ||
|
|
||
| pub fn pool(self: *State) *Node.Pool { | ||
| std.debug.assert(self.pool_rc != null); | ||
| return &self.pool_rc.?.instance; | ||
| } | ||
|
Comment on lines
+36
to
+39
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The style guide (Rule 51) requires asserting function arguments and invariants. Adding an explicit assertion here improves safety and follows the 'fail-fast' principle. References
|
||
|
|
||
| pub fn poolRc(self: *State) *PoolRc { | ||
| std.debug.assert(self.pool_rc != null); | ||
| return self.pool_rc.?; | ||
| } | ||
|
Comment on lines
+41
to
44
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| }; | ||
|
|
||
| pub var state: State = .{}; | ||
|
|
||
| /// JS: pool.ensureCapacity(newSize) | ||
| pub fn ensureCapacity(new_size: js.Number) !void { | ||
| if (!state.initialized) { | ||
| if (state.pool_rc == null) { | ||
| return error.PoolNotInitialized; | ||
| } | ||
|
|
||
| const requested = new_size.assertU32(); | ||
| const old_size = state.pool.nodes.capacity; | ||
| const old_size = state.pool().nodes.capacity; | ||
| if (requested <= old_size) { | ||
| return; | ||
| } | ||
| try state.pool.preheat(@intCast(requested - state.pool.nodes.capacity)); | ||
| try state.pool().preheat(@intCast(requested - state.pool().nodes.capacity)); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| import {spawnSync} from "node:child_process"; | ||
| import {unlinkSync, writeFileSync} from "node:fs"; | ||
| import {join} from "node:path"; | ||
| import {describe, expect, it} from "vitest"; | ||
|
|
||
| describe("BeaconStateView teardown", () => { | ||
| it("creates view at module scope and exits cleanly", () => { | ||
| const projectRoot = join(import.meta.dirname, "../.."); | ||
| // Fixture must live under the project root so Node resolves | ||
| // workspace packages like @lodestar/config from local node_modules. | ||
| const fixturePath = join(projectRoot, `bindings/test/.tmp-teardown-${process.pid}.mjs`); | ||
|
|
||
| writeFileSync( | ||
| fixturePath, | ||
| ` | ||
| import {config} from "@lodestar/config/default"; | ||
| import * as era from "@lodestar/era"; | ||
| import bindings from "../src/index.js"; | ||
| import {getFirstEraFilePath} from "./eraFiles.ts"; | ||
|
|
||
| const reader = await era.era.EraReader.open(config, getFirstEraFilePath()); | ||
| const stateBytes = await reader.readSerializedState(); | ||
| await reader.close(); | ||
|
|
||
| bindings.pool.ensureCapacity(10_000_000); | ||
| bindings.pubkeys.ensureCapacity(2_000_000); | ||
|
|
||
| const seedState = bindings.BeaconStateView.createFromBytes(stateBytes); | ||
| console.log("slot=" + seedState.slot); | ||
| ` | ||
| ); | ||
|
|
||
| try { | ||
| const result = spawnSync(process.execPath, ["--experimental-strip-types", fixturePath], { | ||
| cwd: projectRoot, | ||
| encoding: "utf-8", | ||
| timeout: 60_000, | ||
| }); | ||
| expect(result.status, `stdout=${result.stdout} stderr=${result.stderr}`).toBe(0); | ||
| expect(result.stderr, "no panic on stderr").not.toContain("panic:"); | ||
| } finally { | ||
| try { | ||
| unlinkSync(fixturePath); | ||
| } catch (_e) { | ||
| // ignore | ||
| } | ||
| } | ||
| }, 90_000); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the type reference to
PoolRCto match the updated naming convention inpool.zig.