Skip to content

Commit d134a6a

Browse files
authored
fix!: manage external buffer lifetime (#66)
## Motivation The `.external_buffer` return hint passed borrowed Zig slices to JavaScript without copying or registering a finalizer. JavaScript can retain the Buffer after the native callback returns, leaving it backed by stack, arena, or otherwise freed memory. External buffers should retain their zero-copy behavior while making ownership and cleanup explicit. ## Description - Add an allocator-backed `OwnedBuffer` that transfers its allocation to JavaScript. - Require `.external_buffer` callbacks to return `OwnedBuffer`; ordinary byte-slice returns continue to copy. - Release the backing allocation and finalizer context from the N-API finalizer. - Match napi-rs ownership semantics: conversion consumes `OwnedBuffer`; environments that disallow external buffers copy the bytes and release the original allocation, while other errors preserve ownership already accepted by the N-API finalizer. - Use a normal Buffer for empty data and document the allocator lifetime contract. - Test the public copy contract and finalizer-context allocation failure, plus a real Node `--expose-gc` integration test that verifies the external data and finalizer context remain live while reachable and are both released after GC.
1 parent 748ee2d commit d134a6a

7 files changed

Lines changed: 224 additions & 23 deletions

File tree

‎README.md‎

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -421,18 +421,40 @@ Control how arguments are converted:
421421
```zig
422422
napi.createCallback(2, myFunc, .{
423423
.args = .{ .env, .auto, .value, .data, .string, .buffer },
424-
.returns = .value, // or .string, .buffer, .auto
424+
.returns = .value, // or .string, .buffer, .external_buffer, .auto
425425
});
426426
```
427427

428428
| Hint | Description |
429429
|------|-------------|
430-
| `.auto` | Automatic type conversion |
430+
| `.auto` | Automatic type conversion; byte-slice returns are copied into a JS Buffer |
431431
| `.env` | Inject `napi.Env` |
432432
| `.value` | Pass raw `napi.Value` |
433433
| `.data` | User data pointer passed to createFunction |
434434
| `.string` | Convert to/from `[]const u8` |
435-
| `.buffer` | Convert to/from byte slice |
435+
| `.buffer` | Borrow Buffer arguments as byte slices; copy byte-slice returns |
436+
| `.external_buffer` | Transfer an `OwnedBuffer` to JavaScript without copying |
437+
438+
External buffers require explicit ownership transfer:
439+
440+
```zig
441+
const allocator = std.heap.page_allocator;
442+
443+
fn makeExternalBuffer() !napi.OwnedBuffer {
444+
const data = try allocator.dupe(u8, "external data");
445+
return napi.OwnedBuffer.fromOwnedSlice(allocator, data);
446+
}
447+
448+
const callback = napi.createCallback(0, makeExternalBuffer, .{
449+
.returns = .external_buffer,
450+
});
451+
```
452+
453+
`OwnedBuffer.intoValue` consumes the buffer even if conversion fails, so the caller must not
454+
deinitialize it afterwards. Once N-API accepts the external buffer, the allocator must remain valid
455+
until its finalizer runs, including if N-API subsequently reports an error. If the environment
456+
disallows external buffers, `intoValue` copies the bytes and releases the original allocation before
457+
returning.
436458

437459
### Creating Classes
438460

‎examples/hello_world/mod.test.ts‎

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
1-
import { describe, it, expect } from "vitest";
1+
import { execFileSync } from "node:child_process";
22
import { createRequire } from "node:module";
3+
import { describe, expect, it } from "vitest";
34

45
const require = createRequire(import.meta.url);
5-
const example = require("../../zig-out/lib/example_hello_world.node");
6+
const addonPath = require.resolve("../../zig-out/lib/example_hello_world.node");
7+
const example = require(addonPath);
68

79
describe("example mod", () => {
810
it("addon parameters", () => {
@@ -36,10 +38,48 @@ describe("example mod", () => {
3638
expect(arrayBuffer.byteLength).toEqual(0);
3739
});
3840

39-
it("shares mutable storage with an external Buffer", () => {
41+
it("copies mutable slices returned without an external hint", () => {
42+
const buffer = example.copySlice();
43+
buffer[0] = 9;
44+
expect(example.copySlice()[0]).toEqual(1);
45+
});
46+
47+
it("returns an allocator-owned external Buffer", () => {
4048
const buffer = example.externalBuffer();
4149
expect(Buffer.isBuffer(buffer)).toBe(true);
50+
expect([...buffer]).toEqual([1, 2, 3]);
4251
buffer[0] = 9;
43-
expect(example.externalBufferFirstByte()).toEqual(9);
52+
expect([...buffer]).toEqual([9, 2, 3]);
53+
});
54+
55+
it("releases an external Buffer through its GC finalizer", () => {
56+
const script = `
57+
const addon = require(${JSON.stringify(addonPath)});
58+
const baseline = addon.externalBufferAllocatedBytes();
59+
let buffer = addon.externalBuffer();
60+
if (addon.externalBufferAllocatedBytes() <= baseline) {
61+
throw new Error("external Buffer was released while still reachable");
62+
}
63+
buffer = null;
64+
65+
const deadline = Date.now() + 5000;
66+
function collect() {
67+
global.gc();
68+
const allocatedBytes = addon.externalBufferAllocatedBytes();
69+
if (allocatedBytes === baseline) return;
70+
if (Date.now() >= deadline) {
71+
throw new Error(
72+
\`external Buffer finalizer did not release \${allocatedBytes - baseline} bytes\`,
73+
);
74+
}
75+
setImmediate(collect);
76+
}
77+
setImmediate(collect);
78+
`;
79+
80+
execFileSync(process.execPath, ["--expose-gc", "-e", script], {
81+
stdio: "pipe",
82+
timeout: 10_000,
83+
});
4484
});
4585
});

‎examples/hello_world/mod.zig‎

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,13 @@ fn exampleMod(env: zapi.Env, module: zapi.Value) anyerror!void {
7676
null,
7777
));
7878

79+
try module.setNamedProperty("copySlice", try env.createFunction(
80+
"copySlice",
81+
0,
82+
zapi.createCallback(0, copy_slice, .{}),
83+
null,
84+
));
85+
7986
try module.setNamedProperty("externalBuffer", try env.createFunction(
8087
"externalBuffer",
8188
0,
@@ -85,10 +92,10 @@ fn exampleMod(env: zapi.Env, module: zapi.Value) anyerror!void {
8592
null,
8693
));
8794

88-
try module.setNamedProperty("externalBufferFirstByte", try env.createFunction(
89-
"externalBufferFirstByte",
95+
try module.setNamedProperty("externalBufferAllocatedBytes", try env.createFunction(
96+
"externalBufferAllocatedBytes",
9097
0,
91-
zapi.createCallback(0, external_buffer_first_byte, .{}),
98+
zapi.createCallback(0, external_buffer_allocated_bytes, .{}),
9299
null,
93100
));
94101

@@ -178,14 +185,26 @@ fn copy_buffer(env: zapi.Env, _: zapi.CallbackInfo(0)) !zapi.Value {
178185
return try env.createBufferCopy("copy me", null);
179186
}
180187

181-
var external_buffer_data = [_]u8{ 1, 2, 3 };
188+
const external_buffer_source = [_]u8{ 1, 2, 3 };
189+
190+
var slice_data = [_]u8{ 1, 2, 3 };
191+
var external_buffer_allocator: std.heap.DebugAllocator(.{
192+
.enable_memory_limit = true,
193+
.thread_safe = false,
194+
}) = .init;
195+
196+
fn copy_slice() []u8 {
197+
return &slice_data;
198+
}
182199

183-
fn external_buffer() []u8 {
184-
return &external_buffer_data;
200+
fn external_buffer() !zapi.OwnedBuffer {
201+
const owned_allocator = external_buffer_allocator.allocator();
202+
const data = try owned_allocator.dupe(u8, &external_buffer_source);
203+
return zapi.OwnedBuffer.fromOwnedSlice(owned_allocator, data);
185204
}
186205

187-
fn external_buffer_first_byte() u8 {
188-
return external_buffer_data[0];
206+
fn external_buffer_allocated_bytes() usize {
207+
return external_buffer_allocator.total_requested_bytes;
189208
}
190209

191210
const S = struct {

‎src/OwnedBuffer.zig‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
//! Allocator-backed bytes whose ownership can be transferred to JavaScript.
2+
//! Like other Zig owning values, an OwnedBuffer must not be copied or deinitialized after transfer.
3+
4+
const std = @import("std");
5+
const c = @import("c.zig").c;
6+
const Env = @import("Env.zig");
7+
const Value = @import("Value.zig");
8+
9+
allocator: std.mem.Allocator,
10+
data: []u8,
11+
12+
const OwnedBuffer = @This();
13+
14+
const FinalizerContext = struct {
15+
allocator: std.mem.Allocator,
16+
data: []u8,
17+
};
18+
19+
/// Takes ownership of `data`, which must have been allocated by `allocator`.
20+
/// The allocator must remain valid until the buffer is deinitialized or finalized by JavaScript.
21+
pub fn fromOwnedSlice(allocator: std.mem.Allocator, data: []u8) OwnedBuffer {
22+
return .{
23+
.allocator = allocator,
24+
.data = data,
25+
};
26+
}
27+
28+
/// Copies `data` into a new owned allocation.
29+
pub fn fromSlice(allocator: std.mem.Allocator, data: []const u8) !OwnedBuffer {
30+
return .fromOwnedSlice(allocator, try allocator.dupe(u8, data));
31+
}
32+
33+
/// Releases a buffer that has not been transferred to JavaScript.
34+
pub fn deinit(self: *OwnedBuffer) void {
35+
self.allocator.free(self.data);
36+
self.* = undefined;
37+
}
38+
39+
/// Transfers ownership to a JavaScript Buffer.
40+
/// This consumes the buffer even on failure; the caller must not deinitialize it afterwards.
41+
/// On success, JavaScript releases the allocation through the N-API finalizer.
42+
pub fn intoValue(self: OwnedBuffer, env: Env) !Value {
43+
const allocator = self.allocator;
44+
const data = self.data;
45+
46+
if (data.len == 0) {
47+
defer allocator.free(data);
48+
return try env.createBuffer(0, null);
49+
}
50+
51+
const context = try createFinalizerContext(self);
52+
53+
return env.createExternalBuffer(data, finalize, context) catch |err| {
54+
if (err == error.NoExternalBuffersAllowed) {
55+
defer release(context);
56+
return try env.createBufferCopy(data, null);
57+
}
58+
// Other failures may occur after the finalizer has taken ownership.
59+
return err;
60+
};
61+
}
62+
63+
fn createFinalizerContext(self: OwnedBuffer) !*FinalizerContext {
64+
const context = self.allocator.create(FinalizerContext) catch |err| {
65+
self.allocator.free(self.data);
66+
return err;
67+
};
68+
context.* = .{
69+
.allocator = self.allocator,
70+
.data = self.data,
71+
};
72+
return context;
73+
}
74+
75+
fn finalize(
76+
_: c.napi_env,
77+
finalize_data: ?*anyopaque,
78+
finalize_hint: ?*anyopaque,
79+
) callconv(.c) void {
80+
const context: *FinalizerContext = @ptrCast(@alignCast(finalize_hint orelse unreachable));
81+
std.debug.assert(finalize_data == @as(?*anyopaque, @ptrCast(context.data.ptr)));
82+
release(context);
83+
}
84+
85+
fn release(context: *FinalizerContext) void {
86+
const allocator = context.allocator;
87+
allocator.free(context.data);
88+
allocator.destroy(context);
89+
}
90+
91+
test "OwnedBuffer fromSlice owns an independent copy" {
92+
var source = [_]u8{ 1, 2, 3 };
93+
var buffer = try OwnedBuffer.fromSlice(std.testing.allocator, &source);
94+
defer buffer.deinit();
95+
96+
source[0] = 9;
97+
try std.testing.expectEqualSlices(u8, &.{ 1, 2, 3 }, buffer.data);
98+
}
99+
100+
test "OwnedBuffer releases data when finalizer context allocation fails" {
101+
const data = try std.testing.allocator.dupe(u8, "external");
102+
103+
var failing_allocator = std.testing.FailingAllocator.init(std.testing.allocator, .{
104+
.fail_index = 0,
105+
});
106+
const buffer = OwnedBuffer.fromOwnedSlice(failing_allocator.allocator(), data);
107+
108+
try std.testing.expectError(error.OutOfMemory, createFinalizerContext(buffer));
109+
try std.testing.expectEqual(@as(usize, 1), failing_allocator.deallocations);
110+
}

‎src/napi.zig‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ pub const c = @import("c.zig").c;
44
pub const AsyncContext = @import("AsyncContext.zig");
55
pub const Env = @import("Env.zig");
66
pub const Value = @import("Value.zig");
7+
pub const OwnedBuffer = @import("OwnedBuffer.zig");
78
pub const Deferred = @import("Deferred.zig");
89
pub const EscapableHandleScope = @import("EscapableHandleScope.zig");
910
pub const HandleScope = @import("HandleScope.zig");

‎src/root.zig‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ pub const c = napi.c;
77
pub const AsyncContext = napi.AsyncContext;
88
pub const Env = napi.Env;
99
pub const Value = napi.Value;
10+
pub const OwnedBuffer = napi.OwnedBuffer;
1011
pub const Deferred = napi.Deferred;
1112
pub const EscapableHandleScope = napi.EscapableHandleScope;
1213
pub const HandleScope = napi.HandleScope;

‎src/to_from_value.zig‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
const std = @import("std");
22
const Env = @import("Env.zig");
3+
const OwnedBuffer = @import("OwnedBuffer.zig");
34
const Value = @import("Value.zig");
45

56
pub fn fromValue(
@@ -55,6 +56,21 @@ pub fn toValue(
5556
comptime hint: anytype,
5657
) !Value {
5758
const type_info = @typeInfo(T);
59+
comptime {
60+
const return_type = switch (type_info) {
61+
.error_union => |error_union| error_union.payload,
62+
else => T,
63+
};
64+
if (hint == .external_buffer and return_type != OwnedBuffer) {
65+
@compileError("external_buffer returns require an OwnedBuffer");
66+
}
67+
if (return_type == OwnedBuffer and hint != .external_buffer) {
68+
@compileError("OwnedBuffer returns require the external_buffer hint");
69+
}
70+
}
71+
72+
if (T == OwnedBuffer) return try v.intoValue(env);
73+
5874
switch (type_info) {
5975
.bool => {
6076
return try env.getBoolean(v);
@@ -82,14 +98,6 @@ pub fn toValue(
8298
.pointer => |p| {
8399
const h = hint;
84100
if (p.child == u8 and p.size == .slice) {
85-
if (h == .external_buffer) {
86-
if (p.is_const) {
87-
@compileError("external buffers require a mutable byte slice");
88-
}
89-
const bytes: []u8 = @ptrCast(v);
90-
return try env.createExternalBuffer(bytes, null, null);
91-
}
92-
93101
const bytes: []const u8 = @ptrCast(v);
94102
if (h == .string) {
95103
return try env.createStringUtf8(bytes);

0 commit comments

Comments
 (0)