From 8a1c911e7bb0e4de7883123cebe5174a9be24110 Mon Sep 17 00:00:00 2001 From: Don Isaac Date: Mon, 31 Mar 2025 16:00:33 -0700 Subject: [PATCH] wip: start using Arc(Source) in Blob --- src/bun.js/webcore/blob.zig | 19 ++++++---- src/bun.js/webcore/blob/ReadFile.zig | 47 ++++++++++++------------ src/bun.js/webcore/streams.zig | 15 ++++---- src/ptr/rc.zig | 55 +++++++++++++++++++++++++--- src/shell/shell.zig | 7 ++-- 5 files changed, 98 insertions(+), 45 deletions(-) diff --git a/src/bun.js/webcore/blob.zig b/src/bun.js/webcore/blob.zig index a8a7328d45..8f77a4db62 100644 --- a/src/bun.js/webcore/blob.zig +++ b/src/bun.js/webcore/blob.zig @@ -29,6 +29,7 @@ const JSPromise = JSC.JSPromise; const JSValue = JSC.JSValue; const JSGlobalObject = JSC.JSGlobalObject; const NullableAllocator = bun.NullableAllocator; +const Arc = bun.ptr.Arc; const VirtualMachine = JSC.VirtualMachine; const Task = JSC.Task; @@ -83,7 +84,7 @@ pub const Blob = struct { /// When set, the blob will be freed on finalization callbacks /// If the blob is contained in Response or Request, this must be null allocator: ?std.mem.Allocator = null, - store: ?*Store = null, + store: ?Arc(Store) = null, content_type: string = "", content_type_allocated: bool = false, content_type_was_set: bool = false, @@ -124,7 +125,10 @@ pub const Blob = struct { } pub fn hasContentTypeFromUser(this: *const Blob) bool { - return this.content_type_was_set or (this.store != null and (this.store.?.data == .file or this.store.?.data == .s3)); + return this.content_type_was_set or if (this.store) |store| switch (store.get().data) { + .s3, .file => true, + else => false, + } else false; } pub fn contentTypeOrMimeType(this: *const Blob) ?[]const u8 { @@ -132,7 +136,7 @@ pub const Blob = struct { return this.content_type; } if (this.store) |store| { - switch (store.data) { + switch (store.get().data) { .file => |file| { return file.mime_type.value; }, @@ -148,7 +152,7 @@ pub const Blob = struct { pub fn isBunFile(this: *const Blob) bool { const store = this.store orelse return false; - return store.data == .file; + return store.get().data == .file; } pub fn doReadFromS3(this: *Blob, comptime Function: anytype, global: *JSGlobalObject) JSValue { @@ -5183,7 +5187,7 @@ pub const Blob = struct { return tryCreate(bytes_, allocator_, globalThis, was_string) catch bun.outOfMemory(); } - pub fn initWithStore(store: *Blob.Store, globalThis: *JSGlobalObject) Blob { + pub fn initWithStore(store: Arc(Blob.Store), globalThis: *JSGlobalObject) Blob { return Blob{ .size = store.size(), .store = store, @@ -5213,7 +5217,7 @@ pub const Blob = struct { } pub fn detach(this: *Blob) void { - if (this.store != null) this.store.?.deref(); + if (this.store) |*s| s.deinit(); this.store = null; } @@ -5225,7 +5229,8 @@ pub const Blob = struct { } pub fn dupeWithContentType(this: *const Blob, include_content_type: bool) Blob { - if (this.store != null) this.store.?.ref(); + // SAFETY: creating a copy of `this`, so the new reference belongs to the new Blob. + if (this.store) |*s| s.dangerouslyIncrementRef(); var duped = this.*; if (duped.content_type_allocated and duped.allocator != null and !include_content_type) { diff --git a/src/bun.js/webcore/blob/ReadFile.zig b/src/bun.js/webcore/blob/ReadFile.zig index 040ff469b4..f8cea782ed 100644 --- a/src/bun.js/webcore/blob/ReadFile.zig +++ b/src/bun.js/webcore/blob/ReadFile.zig @@ -15,6 +15,7 @@ const JSPromise = JSC.JSPromise; const JSGlobalObject = JSC.JSGlobalObject; const ZigString = JSC.ZigString; const libuv = bun.windows.libuv; +const Arc = bun.ptr.Arc; const log = bun.Output.scoped(.ReadFile, true); @@ -64,7 +65,7 @@ pub const ReadFileTask = JSC.WorkTask(ReadFile); pub const ReadFile = struct { file_store: FileStore, byte_store: ByteStore = ByteStore{ .allocator = bun.default_allocator }, - store: ?*Store = null, + store: ?Arc(Store) = null, offset: SizeType = 0, max_length: SizeType = Blob.max_size, total_size: SizeType = Blob.max_size, @@ -100,7 +101,7 @@ pub const ReadFile = struct { pub fn createWithCtx( _: std.mem.Allocator, - store: *Store, + store: Arc(Store), onReadFileContext: *anyopaque, onCompleteCallback: ReadFileOnReadFileCallback, off: SizeType, @@ -110,20 +111,20 @@ pub const ReadFile = struct { @compileError("Do not call ReadFile.createWithCtx on Windows, see ReadFileUV"); const read_file = bun.new(ReadFile, ReadFile{ - .file_store = store.data.file, + // TODO: do not store this separately from .store + .file_store = store.get().data.file, .offset = off, .max_length = max_len, .store = store, .onCompleteCtx = onReadFileContext, .onCompleteCallback = onCompleteCallback, }); - store.ref(); return read_file; } pub fn create( allocator: std.mem.Allocator, - store: *Store, + store: Arc(Store), off: SizeType, max_len: SizeType, comptime Context: type, @@ -258,14 +259,14 @@ pub const ReadFile = struct { const cb = this.onCompleteCallback; const cb_ctx = this.onCompleteCtx; - if (this.store == null and this.system_error != null) { - const system_error = this.system_error.?; - bun.destroy(this); - cb(cb_ctx, ReadFileResultType{ .err = system_error }); - return; - } else if (this.store == null) { - bun.destroy(this); + var store = this.store orelse { + if (this.system_error) |system_error| { + bun.destroy(this); + cb(cb_ctx, ReadFileResultType{ .err = system_error }); + return; + } if (Environment.allow_assert) @panic("assertion failure - store should not be null"); + bun.destroy(this); cb(cb_ctx, ReadFileResultType{ .err = SystemError{ .code = bun.String.static("INTERNAL_ERROR"), @@ -274,12 +275,11 @@ pub const ReadFile = struct { }, }); return; - } + }; - var store = this.store.?; const buf = this.buffer.items; - defer store.deref(); + defer store.deinit(); const system_error = this.system_error; const total_size = this.total_size; bun.destroy(this); @@ -342,8 +342,9 @@ pub const ReadFile = struct { }; if (this.store) |store| { - if (store.data == .file) { - store.data.file.last_modified = JSC.toJSTime(stat.mtime().sec, stat.mtime().nsec); + const data = &store.getMut().data; + if (data == .file) { + data.file.last_modified = JSC.toJSTime(stat.mtime().sec, stat.mtime().nsec); } } @@ -547,7 +548,7 @@ pub const ReadFileUV = struct { loop: *libuv.Loop, file_store: FileStore, byte_store: ByteStore = ByteStore{ .allocator = bun.default_allocator }, - store: *Store, + store: Arc(Store), offset: SizeType = 0, max_length: SizeType = Blob.max_size, total_size: SizeType = Blob.max_size, @@ -565,7 +566,7 @@ pub const ReadFileUV = struct { req: libuv.fs_t = std.mem.zeroes(libuv.fs_t), - pub fn start(loop: *libuv.Loop, store: *Store, off: SizeType, max_len: SizeType, comptime Handler: type, handler: *anyopaque) void { + pub fn start(loop: *libuv.Loop, store: Arc(Store), off: SizeType, max_len: SizeType, comptime Handler: type, handler: *anyopaque) void { log("ReadFileUV.start", .{}); var this = bun.new(ReadFileUV, .{ .loop = loop, @@ -576,14 +577,13 @@ pub const ReadFileUV = struct { .on_complete_data = handler, .on_complete_fn = @ptrCast(&Handler.run), }); - store.ref(); this.getFd(onFileOpen); } pub fn finalize(this: *ReadFileUV) void { log("ReadFileUV.finalize", .{}); defer { - this.store.deref(); + this.store.deinit(); this.req.deinit(); bun.destroy(this); log("ReadFileUV.finalize destroy", .{}); @@ -657,8 +657,9 @@ pub const ReadFileUV = struct { log("stat: {any}", .{stat}); // keep in sync with resolveSizeAndLastModified - if (this.store.data == .file) { - this.store.data.file.last_modified = JSC.toJSTime(stat.mtime().sec, stat.mtime().nsec); + const data = &this.store.getMut().data; + if (data == .file) { + data.file.last_modified = JSC.toJSTime(stat.mtime().sec, stat.mtime().nsec); } if (bun.S.ISDIR(@intCast(stat.mode))) { diff --git a/src/bun.js/webcore/streams.zig b/src/bun.js/webcore/streams.zig index 9cbf8fe9cb..ace0ba5313 100644 --- a/src/bun.js/webcore/streams.zig +++ b/src/bun.js/webcore/streams.zig @@ -44,6 +44,8 @@ const Syscall = bun.sys; const uv = bun.windows.libuv; const AnyBlob = bun.JSC.WebCore.AnyBlob; +const Arc = bun.ptr.Arc; + pub const ReadableStream = struct { value: JSValue, ptr: Source, @@ -339,7 +341,7 @@ pub const ReadableStream = struct { var store = blob.store orelse { return ReadableStream.empty(globalThis); }; - switch (store.data) { + switch (store.getMut().data) { .bytes => { var reader = ByteBlobLoader.Source.new( .{ @@ -359,11 +361,10 @@ pub const ReadableStream = struct { .max_size = if (blob.size != Blob.max_size) blob.size else null, .lazy = .{ - .blob = store, + .blob = store.clone(), }, }, }); - store.ref(); return reader.toReadableStream(globalThis); }, @@ -373,6 +374,7 @@ pub const ReadableStream = struct { const proxy = globalThis.bunVM().transpiler.env.getHttpProxy(true, null); const proxy_url = if (proxy) |p| p.href else null; + // NOTE: contains a reference to blob store field, but store isn't ref'd. This smells like a bug. return bun.S3.readableStream(credentials, path, blob.offset, if (blob.size != Blob.max_size) blob.size else null, proxy_url, globalThis); }, } @@ -387,7 +389,7 @@ pub const ReadableStream = struct { var store = blob.store orelse { return ReadableStream.empty(globalThis); }; - switch (store.data) { + switch (store.getMut().data) { .file => { var reader = FileReader.Source.new(.{ .globalThis = globalThis, @@ -395,11 +397,10 @@ pub const ReadableStream = struct { .event_loop = JSC.EventLoopHandle.init(globalThis.bunVM().eventLoop()), .start_offset = offset, .lazy = .{ - .blob = store, + .blob = store.clone(), }, }, }); - store.ref(); return reader.toReadableStream(globalThis); }, @@ -4091,7 +4092,7 @@ pub const FileReader = struct { pub const Lazy = union(enum) { none: void, - blob: *Blob.Store, + blob: Arc(Blob.Store), const OpenedFileBlob = struct { fd: bun.FileDescriptor, diff --git a/src/ptr/rc.zig b/src/ptr/rc.zig index 80d8691954..0bb1c0ed7a 100644 --- a/src/ptr/rc.zig +++ b/src/ptr/rc.zig @@ -97,7 +97,7 @@ pub fn RefCounted( ) type { const output_name: [:0]const u8 = if (!bun.Environment.enable_logs) - &"" + "" else if (@hasDecl(Context, "debug_name") and Context.debug_name != null) Context.debug_name else @@ -109,14 +109,14 @@ pub fn RefCounted( const Inner = struct { /// This is always > 0 unless value is just about to be deallocated. - ref_count: if (intrusive) void else if (sync) AtomicU32 else u32 = if (sync) .init(1) else 1, + ref_count: if (intrusive) void else if (sync) AtomicU32 else u32 = if (intrusive) {} else if (sync) .init(1) else 1, allocator: Allocator, value: T, const Self = @This(); fn ref(this: *Self) callconv(bun.callconv_inline) u32 { - const ref_count_field = @field(if (intrusive) this.value else this, "ref_count"); + var ref_count_field = @field(if (intrusive) this.value else this, "ref_count"); if (comptime sync) { const prev = ref_count_field.fetchAdd(1, AtomicOrder.acquire); bun.assertWithLocation(prev > 0, @src()); @@ -130,7 +130,7 @@ pub fn RefCounted( } fn deref(this: *Self) callconv(bun.callconv_inline) u32 { - const ref_count_field = @field(if (intrusive) this.value else this, "ref_count"); + var ref_count_field = @field(if (intrusive) this.value else this, "ref_count"); if (comptime sync) { const prev = ref_count_field.fetchSub(1, AtomicOrder.release); bun.assertWithLocation(prev > 0, @src()); @@ -190,14 +190,59 @@ pub fn RefCounted( return this; } - pub fn deinit(this: Self) void { + /// Increment the reference count without obtaining a new pointer. + /// + /// While this does not return a new pointer, it _does_ create an owned + /// reference. Caller must ensure the new reference's lifecycle is + /// properly managed (that is, its owner calls `.deinit()`), otherwise a + /// memory leak will occur. + pub fn dangerouslyIncrementRef(this: Self) void { + std.mem.doNotOptimizeAway(this.clone()); + } + + /// Decrement the reference count without deinitializing the pointer. + /// When the reference count hits 0, the value is deinitialized and the + /// allocation is freed. + /// + /// ## Safety + /// Decrementing the reference count below 1 while holding a live + /// reference count is illegal behavior. This pointer's owner must die + /// at the same time as this pointer, otherwise it may hold a dangling + /// pointer. + /// + /// Caller should set `ensure_alive` to `true` when it assumes the + /// allocation will continue to be alive after this call. This enables + /// runtime safety checks and is useful, for example, when transferring + /// ownership across threads or Zig/c++ boundaries. + pub fn dangerouslyDecrementRef(this: Self, comptime ensure_alive: bool) void { const prev = this.__ptr.deref(); log("0x{x} deref {d} - 1 = {d}", .{ @intFromPtr(this.__ptr), prev, prev - 1 }); if (prev == 1) { @branchHint(.unlikely); + if (comptime ensure_alive) bun.assert(false); + // SAFETY: caller ensures that + // 1. owner will die at the same time as this pointer, meaning nobody owns the dangling pointer + // 2. reference count is not decremented below 1, meaning the pointer is not dangling + this.destroy(); } } + /// Deinitialize the pointer. + /// + /// `deinit` decrements the stored allocation's reference count. If that + /// count hits 0, the value gets deinitialized and the allocation is freed. + pub fn deinit(this: *Self) void { + const prev = this.__ptr.deref(); + log("0x{x} deref {d} - 1 = {d}", .{ @intFromPtr(this.__ptr), prev, prev - 1 }); + if (prev == 1) { + @branchHint(.unlikely); + this.destroy(); + } + // pointer cannot be used after .deinit(), even if other references + // to the allocation still exist + this.* = undefined; + } + fn destroy(this: Self) void { const allocator = this.__ptr.allocator; diff --git a/src/shell/shell.zig b/src/shell/shell.zig index 4c6e1cd088..ac3d5f4cce 100644 --- a/src/shell/shell.zig +++ b/src/shell/shell.zig @@ -3778,9 +3778,10 @@ pub fn handleTemplateValue( if (template_value.as(JSC.WebCore.Blob)) |blob| { if (blob.store) |store| { - if (store.data == .file) { - if (store.data.file.pathlike == .path) { - const path = store.data.file.pathlike.path.slice(); + const s = store.get(); + if (s.data == .file) { + if (s.data.file.pathlike == .path) { + const path = s.data.file.pathlike.path.slice(); if (!try builder.appendUTF8(path, true)) { return globalThis.throw("Shell script string contains invalid UTF-16", .{}); }