Skip to content

Commit 75b982f

Browse files
committed
fix: finalize placeholder instances in class factory wrappers
1 parent cbc1e15 commit 75b982f

File tree

3 files changed

+80
-5
lines changed

3 files changed

+80
-5
lines changed

examples/js_dsl/mod.test.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,31 @@ describe("optional parameters", () => {
272272
});
273273
});
274274

275+
describe("factory cleanup", () => {
276+
it("static factory deinitializes constructor placeholder", () => {
277+
const initBefore = mod.getFactoryResourceInitCount();
278+
const deinitBefore = mod.getFactoryResourceDeinitCount();
279+
280+
const resource = mod.FactoryResource.withByte(7);
281+
282+
expect(resource.getByte()).toEqual(7);
283+
expect(mod.getFactoryResourceInitCount()).toEqual(initBefore + 2);
284+
expect(mod.getFactoryResourceDeinitCount()).toEqual(deinitBefore + 1);
285+
});
286+
287+
it("instance factory deinitializes constructor placeholder", () => {
288+
const base = mod.FactoryResource.withByte(1);
289+
const initBefore = mod.getFactoryResourceInitCount();
290+
const deinitBefore = mod.getFactoryResourceDeinitCount();
291+
292+
const clone = base.cloneWithByte(9);
293+
294+
expect(clone.getByte()).toEqual(9);
295+
expect(mod.getFactoryResourceInitCount()).toEqual(initBefore + 2);
296+
expect(mod.getFactoryResourceDeinitCount()).toEqual(deinitBefore + 1);
297+
});
298+
});
299+
275300
// Section 15: Getters and Setters
276301
describe("Settings class (getters/setters)", () => {
277302
it("has getter for volume with default value", () => {

examples/js_dsl/mod.zig

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -308,6 +308,24 @@ pub fn getEnvRefcount() Number {
308308
// Section 13: Static Factory Methods + Optional Parameters
309309
// ============================================================================
310310

311+
var factory_resource_init_count: std.atomic.Value(u32) = std.atomic.Value(u32).init(0);
312+
var factory_resource_deinit_count: std.atomic.Value(u32) = std.atomic.Value(u32).init(0);
313+
314+
fn makeFactoryResource(byte: u8) !FactoryResource {
315+
const data = try js.allocator().alloc(u8, 1);
316+
data[0] = byte;
317+
_ = factory_resource_init_count.fetchAdd(1, .monotonic);
318+
return .{ .data = data };
319+
}
320+
321+
pub fn getFactoryResourceInitCount() Number {
322+
return Number.from(@as(i32, @intCast(factory_resource_init_count.load(.acquire))));
323+
}
324+
325+
pub fn getFactoryResourceDeinitCount() Number {
326+
return Number.from(@as(i32, @intCast(factory_resource_deinit_count.load(.acquire))));
327+
}
328+
311329
/// A point class demonstrating static factories and optional params.
312330
pub const Point = struct {
313331
pub const js_class = true;
@@ -349,6 +367,34 @@ pub const Point = struct {
349367
}
350368
};
351369

370+
/// A resource-owning class used to verify placeholder cleanup in factory paths.
371+
pub const FactoryResource = struct {
372+
pub const js_class = true;
373+
data: []u8,
374+
375+
pub fn init() !FactoryResource {
376+
return makeFactoryResource(0);
377+
}
378+
379+
pub fn withByte(value: Number) !FactoryResource {
380+
return makeFactoryResource(@intCast(value.assertU32()));
381+
}
382+
383+
pub fn cloneWithByte(self: FactoryResource, value: Number) !FactoryResource {
384+
_ = self;
385+
return makeFactoryResource(@intCast(value.assertU32()));
386+
}
387+
388+
pub fn getByte(self: FactoryResource) Number {
389+
return Number.from(@as(i32, @intCast(self.data[0])));
390+
}
391+
392+
pub fn deinit(self: *FactoryResource) void {
393+
_ = factory_resource_deinit_count.fetchAdd(1, .monotonic);
394+
js.allocator().free(self.data);
395+
}
396+
};
397+
352398
// ============================================================================
353399
// Module Export
354400
// ============================================================================

src/js/wrap_class.zig

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -49,14 +49,18 @@ pub fn wrapClass(comptime T: type) type {
4949
/// C-ABI constructor callback for napi_define_class.
5050
pub const constructor: napi.c.napi_callback = genConstructor();
5151

52-
/// Default GC-invoked destructor. Frees the native object using c_allocator.
53-
pub fn defaultFinalize(_: napi.Env, obj: *T, _: ?*anyopaque) void {
52+
fn destroyNativeObject(obj: *T) void {
5453
if (@hasDecl(T, "deinit")) {
5554
obj.deinit();
5655
}
5756
std.heap.c_allocator.destroy(obj);
5857
}
5958

59+
/// Default GC-invoked destructor. Frees the native object using c_allocator.
60+
pub fn defaultFinalize(_: napi.Env, obj: *T, _: ?*anyopaque) void {
61+
destroyNativeObject(obj);
62+
}
63+
6064
fn isClassSelfParam(comptime ParamType: type) bool {
6165
return ParamType == *T or ParamType == *const T or ParamType == T;
6266
}
@@ -620,7 +624,7 @@ pub fn wrapClass(comptime T: type) type {
620624
e.throwError("", "Failed to remove constructor wrap in instance factory") catch {};
621625
return null;
622626
};
623-
std.heap.c_allocator.destroy(old_ptr);
627+
destroyNativeObject(old_ptr);
624628

625629
// Wrap with the factory's result
626630
_ = e.wrap(instance_napi, Class, obj_ptr, defaultFinalize, null, null) catch {
@@ -837,8 +841,8 @@ pub fn wrapClass(comptime T: type) type {
837841
e.throwError("", "Failed to remove constructor wrap in factory") catch {};
838842
return null;
839843
};
840-
// The constructor allocated this — free it since we're replacing
841-
std.heap.c_allocator.destroy(old_ptr);
844+
// The constructor allocated this — destroy it via the same path as GC finalization.
845+
destroyNativeObject(old_ptr);
842846

843847
// Wrap with the factory's result
844848
_ = e.wrap(instance_val, Class, obj_ptr, defaultFinalize, null, null) catch {

0 commit comments

Comments
 (0)