From 2eab4d2630f5539c41373fb0665cee7ea1dc1808 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Mon, 6 Jul 2026 13:05:37 +0800 Subject: [PATCH] refactor: HttpClient Replaces layering with an inline request pipeline, and transfer queue. This is meant to simplify the code, reduce footguns, and make future enhancements easier to implement (e.g. speculative parsing (which requires streaming to fully leverage)). Previously, HttpClient implemented deferring as a layer which required special pumping at various callsites (https://github.com/lightpanda-io/browser/pull/2855, https://github.com/lightpanda-io/browser/pull/2843, ...). In this new approach, deferring is built-into the HttpClient/Transfer's flow. Specifically, Transfers now maintain a queue of events (start, header, data, end, err) which are dispatched in HttpClient.tick. The result is that JS callbacks are never executed in the same stack that initiated the I/O, without needing guards or any external intervention. tTwo other benefits come from this. The first is that reentrant libcurl is eliminated. Instead of "libcurl -> callback", it's now "libcurl -> transfer event queue THEN tick -> callback" (we don't have to wait until the NEXT tick, we can just do it later in the tick). HttpClient still has to guard against libcurl reentrancy, but only because of how WebSocket is implemented, and we should be able to unify WebSockets to use an event queue too in a follow up PR (which will eliminate a bunch of guard code). The transfer queue should also be useful to re-implement streaming, since a data chunk is just an event in the transfer's event queue. For now, I kept it as a single buffered event to minimize the change. But since speculative parsing depends on this, and speculative parsing seems to be the next major performance tweak we can make, we need to re-introduce streaming. The other change is the removal of all other layers in favor of a pipeline. This works well with the existing Transfer.park mechanism, where a parked Transfer can restart the pipeline for a transfer in an arbitrary point (not as fancy as it sounds given how simple the flow is). The fallout from this is that we're no longer creating/wrapping contexts and callbacks: whatever the request was configured with is all we need. Because of this, HttpClient.Response is removed. There are no intermediary responses and no changing context, everything is just the Transfer. A smaller change is the addition of newRequest + transfer.submit(). The one-shot HttpClient.request and HttpClient.requestT still exist, but this explicit create + submit has some advantage. First, callers can use the transfer.arena (e.g. Frame using the transfer's arena to set the Referrer header). Second, callers can holds Transfer immediately, rather than waiting for their startCallback to be fired. An abort on an XMLHttpRequest called before the start of the transfer no longer silently fails. --- src/Notification.zig | 4 +- src/browser/Browser.zig | 2 +- src/browser/Frame.zig | 129 +- src/browser/Runner.zig | 8 +- src/browser/ScriptManager.zig | 9 +- src/browser/ScriptManagerBase.zig | 100 +- src/browser/js/Execution.zig | 9 +- src/browser/webapi/Worker.zig | 51 +- src/browser/webapi/WorkerGlobalScope.zig | 10 +- src/browser/webapi/element/html/Link.zig | 7 +- src/browser/webapi/net/Fetch.zig | 61 +- src/browser/webapi/net/Response.zig | 10 +- src/browser/webapi/net/WebSocket.zig | 2 +- src/browser/webapi/net/XMLHttpRequest.zig | 76 +- src/cdp/CDP.zig | 10 +- src/cdp/domains/fetch.zig | 6 +- src/cdp/domains/network.zig | 56 +- src/cdp/domains/page.zig | 4 +- src/cdp/id.zig | 2 +- src/lightpanda.zig | 2 +- src/{browser => network}/HttpClient.zig | 2171 ++++++++++++--------- src/network/RobotsGate.zig | 232 +++ src/network/http.zig | 7 +- src/network/layer/CacheLayer.zig | 386 ---- src/network/layer/DeferringLayer.zig | 361 ---- src/network/layer/Forward.zig | 76 - src/network/layer/InterceptionLayer.zig | 327 ---- src/network/layer/RobotsLayer.zig | 291 --- src/network/layer/WebBotAuthLayer.zig | 43 - 29 files changed, 1772 insertions(+), 2680 deletions(-) rename src/{browser => network}/HttpClient.zig (63%) create mode 100644 src/network/RobotsGate.zig delete mode 100644 src/network/layer/CacheLayer.zig delete mode 100644 src/network/layer/DeferringLayer.zig delete mode 100644 src/network/layer/Forward.zig delete mode 100644 src/network/layer/InterceptionLayer.zig delete mode 100644 src/network/layer/RobotsLayer.zig delete mode 100644 src/network/layer/WebBotAuthLayer.zig diff --git a/src/Notification.zig b/src/Notification.zig index 4a96b2382..c28a74c85 100644 --- a/src/Notification.zig +++ b/src/Notification.zig @@ -21,8 +21,7 @@ const lp = @import("lightpanda"); const js = @import("browser/js/js.zig"); const Frame = @import("browser/Frame.zig"); -const Transfer = @import("browser/HttpClient.zig").Transfer; -const Response = @import("browser/HttpClient.zig").Response; +const Transfer = @import("network/HttpClient.zig").Transfer; const log = lp.log; const Execution = js.Execution; @@ -224,7 +223,6 @@ pub const ResponseData = struct { pub const ResponseHeaderDone = struct { transfer: *Transfer, - response: *const Response, }; pub const RequestDone = struct { diff --git a/src/browser/Browser.zig b/src/browser/Browser.zig index e79834aa0..b767140ed 100644 --- a/src/browser/Browser.zig +++ b/src/browser/Browser.zig @@ -28,7 +28,7 @@ const Watchdog = @import("../Watchdog.zig"); const Session = @import("Session.zig"); const Selector = @import("webapi/selector/Selector.zig"); const Viewport = @import("Viewport.zig"); -const HttpClient = @import("HttpClient.zig"); +const HttpClient = @import("../network/HttpClient.zig"); const PermissionState = @import("webapi/Permissions.zig").State; const ArenaPool = App.ArenaPool; diff --git a/src/browser/Frame.zig b/src/browser/Frame.zig index 23c2047c8..aa51e6eb6 100644 --- a/src/browser/Frame.zig +++ b/src/browser/Frame.zig @@ -60,7 +60,7 @@ const popover = @import("webapi/element/popover.zig"); const slotting = @import("webapi/element/slotting.zig"); const NavigationKind = @import("webapi/navigation/root.zig").NavigationKind; -const HttpClient = @import("HttpClient.zig"); +const HttpClient = @import("../network/HttpClient.zig"); const sys_url = @import("../sys/url.zig"); const timestamp = @import("../datetime.zig").timestamp; @@ -597,16 +597,16 @@ pub fn navigate(self: *Frame, request_url: [:0]const u8, opts: NavigateOpts) !vo self._load_state = .parsing; self._last_navigate_error = null; - const req_id = self._session.browser.http_client.nextReqId(); log.info(.frame, "navigate", .{ .url = request_url, .method = opts.method, .reason = opts.reason, .body = opts.body != null, - .req_id = req_id, .type = self._type, }); + const http_client = &session.browser.http_client; + // Handle synthetic navigations: about:blank and blob: URLs const is_about_blank = std.mem.eql(u8, "about:blank", request_url); const is_blob = !is_about_blank and std.mem.startsWith(u8, request_url, "blob:"); @@ -670,6 +670,10 @@ pub fn navigate(self: *Frame, request_url: [:0]const u8, opts: NavigateOpts) !vo }; } + // No real request is made, but CDP still correlates the navigation + // events by request id, so consume one. + const req_id = http_client.incrReqId(); + session.notification.dispatch(.frame_navigate, &.{ .opts = opts, .req_id = req_id, @@ -694,9 +698,6 @@ pub fn navigate(self: *Frame, request_url: [:0]const u8, opts: NavigateOpts) !vo .timestamp = timestamp(.monotonic), }); - // force next request id manually b/c we won't create a real req. - _ = session.browser.http_client.incrReqId(); - if (self.parent == null) { session.navigation._current_navigation_kind = opts.kind; try session.navigation.commitNavigation(self); @@ -706,8 +707,6 @@ pub fn navigate(self: *Frame, request_url: [:0]const u8, opts: NavigateOpts) !vo return; } - const http_client = &session.browser.http_client; - self._http_status = null; self._http_headers = .empty; @@ -719,7 +718,6 @@ pub fn navigate(self: *Frame, request_url: [:0]const u8, opts: NavigateOpts) !vo }; self.origin = try URL.getOrigin(self.arena, self.url); - self._req_id = req_id; self._navigated_options = .{ .cdp_id = opts.cdp_id, .reason = opts.reason, @@ -728,14 +726,37 @@ pub fn navigate(self: *Frame, request_url: [:0]const u8, opts: NavigateOpts) !vo .header = if (opts.header) |h| try self.arena.dupeZ(u8, h) else null, }; - var headers = try http_client.newHeaders(); - try headers.add(lp.Config.HttpHeaders.navigation_accept); - if (opts.header) |hdr| { - try headers.add(hdr); - } - if (opts.referer) |ref| { - const ref_header = try std.mem.concatWithSentinel(self.arena, u8, &.{ "Referer: ", ref }, 0); - try headers.add(ref_header); + const transfer = try http_client.newRequest(.{ + .ctx = self, + .url = self.url, + .frame_id = self._frame_id, + .loader_id = self._loader_id, + .method = opts.method, + .body = opts.body, + .cookie_jar = &session.cookie_jar, + .cookie_origin = opts.initiator_url orelse self.url, + .resource_type = .document, + .notification = self._session.notification, + .header_callback = frameHeaderDoneCallback, + .data_callback = frameDataCallback, + .done_callback = frameDoneCallback, + .error_callback = frameErrorCallback, + // The frame tracks its navigation by id (_req_id), never by pointer. + .shutdown_callback = HttpClient.noopShutdown, + }, &self._http_owner); + self._req_id = transfer.id; + + { + // Ours until submit; clean up if header setup fails. + errdefer transfer.deinit(); + try transfer.req.headers.add(lp.Config.HttpHeaders.navigation_accept); + if (opts.header) |hdr| { + try transfer.req.headers.add(hdr); + } + if (opts.referer) |ref| { + const ref_header = try std.mem.concatWithSentinel(transfer.arena, u8, &.{ "Referer: ", ref }, 0); + try transfer.req.headers.add(ref_header); + } } // A root navigation issued against a pending Page (i.e. one allocated by @@ -751,7 +772,7 @@ pub fn navigate(self: *Frame, request_url: [:0]const u8, opts: NavigateOpts) !vo session.notification.dispatch(.frame_navigate, &.{ .opts = opts, .url = self.url, - .req_id = req_id, + .req_id = transfer.id, .frame_id = self._frame_id, .loader_id = self._loader_id, .timestamp = timestamp(.monotonic), @@ -763,23 +784,7 @@ pub fn navigate(self: *Frame, request_url: [:0]const u8, opts: NavigateOpts) !vo session.navigation._current_navigation_kind = opts.kind; - self.makeRequest(.{ - .ctx = self, - .url = self.url, - .frame_id = self._frame_id, - .loader_id = self._loader_id, - .method = opts.method, - .headers = headers, - .body = opts.body, - .cookie_jar = &session.cookie_jar, - .cookie_origin = opts.initiator_url orelse self.url, - .resource_type = .document, - .notification = self._session.notification, - .header_callback = frameHeaderDoneCallback, - .data_callback = frameDataCallback, - .done_callback = frameDoneCallback, - .error_callback = frameErrorCallback, - }) catch |err| { + transfer.submit() catch |err| { log.err(.frame, "navigate request", .{ .url = self.url, .err = err, .type = self._type }); return err; }; @@ -962,6 +967,11 @@ pub fn makeRequest(self: *Frame, req: HttpClient.Request) !void { return self._session.browser.http_client.request(req, &self._http_owner); } +// Two-phase variant; see HttpClient.newRequest for the ownership contract. +pub fn newRequest(self: *Frame, req: HttpClient.Request) !*HttpClient.Transfer { + return self._session.browser.http_client.newRequest(req, &self._http_owner); +} + // Synchronously abort every transfer and WebSocket owned by this frame // and all of its descendants. pub fn abortTransfers(self: *Frame) void { @@ -970,8 +980,6 @@ pub fn abortTransfers(self: *Frame) void { } const http_client = &self._session.browser.http_client; http_client.abortOwner(&self._http_owner); - // abortOwner misses deferred contexts whose transfer already completed. - http_client.deferring_layer.cancelFrame(self._frame_id); } pub fn documentIsLoaded(self: *Frame) void { @@ -1144,8 +1152,8 @@ fn notifyParentLoadComplete(self: *Frame) void { parent.iframeCompletedLoading(self.iframe.?, self._delays_parent_load); } -fn frameHeaderDoneCallback(response: HttpClient.Response) !HttpClient.HeaderResult { - var self: *Frame = @ptrCast(@alignCast(response.ctx)); +fn frameHeaderDoneCallback(transfer: *HttpClient.Transfer) !HttpClient.Transfer.HeaderResult { + var self: *Frame = @ptrCast(@alignCast(transfer.req.ctx)); // Commit point for a pending root navigation. The session has been // holding the OLD page alive during the round-trip; now that response @@ -1157,7 +1165,7 @@ fn frameHeaderDoneCallback(response: HttpClient.Response) !HttpClient.HeaderResu try self._session.commitPendingPage(self._page); } - const response_url = response.url(); + const response_url = transfer.req.url; if (std.mem.eql(u8, response_url, self.url) == false) { // would be different than self.url in the case of a redirect self.url = try self.arena.dupeZ(u8, response_url); @@ -1169,7 +1177,7 @@ fn frameHeaderDoneCallback(response: HttpClient.Response) !HttpClient.HeaderResu // Page.reload doesn't re-POST form data to the redirect target. Conservative // default — 307/308 technically preserve the method per RFC 7231, but // resubmitting form data is the more dangerous failure mode. - if ((response.redirectCount() orelse 0) > 0) { + if ((transfer.redirectCount() orelse 0) > 0) { if (self._navigated_options) |*no| { no.method = .GET; no.body = null; @@ -1186,14 +1194,14 @@ fn frameHeaderDoneCallback(response: HttpClient.Response) !HttpClient.HeaderResu if (comptime IS_DEBUG) { log.debug(.frame, "navigate header", .{ .url = self.url, - .status = response.status(), - .content_type = response.contentType(), + .status = transfer.responseStatus(), + .content_type = transfer.contentType(), .type = self._type, }); } - self._http_status = response.status(); - var it = response.headerIterator(); + self._http_status = transfer.responseStatus(); + var it = transfer.responseHeaderIterator(); while (it.next()) |hdr| { try self._http_headers.append(self.arena, .{ .name = try self.arena.dupe(u8, hdr.name), @@ -1218,7 +1226,7 @@ fn frameHeaderDoneCallback(response: HttpClient.Response) !HttpClient.HeaderResu // If the response is a file download, stream its body to disk instead of // parsing it as a page. This sets _parse_state to .download, which the // data/done callbacks below special-case. - _ = try self.maybeStartDownload(response); + _ = try self.maybeStartDownload(transfer); return .proceed; } @@ -1227,7 +1235,7 @@ fn frameHeaderDoneCallback(response: HttpClient.Response) !HttpClient.HeaderResu // treated as a download when Browser.setDownloadBehavior opted in // (allow/allowAndName) and the response carries Content-Disposition: attachment. // See issue #2701. -fn maybeStartDownload(self: *Frame, response: HttpClient.Response) !bool { +fn maybeStartDownload(self: *Frame, transfer: *HttpClient.Transfer) !bool { const session = self._session; switch (session.download_behavior) { .allow, .allow_and_name => {}, @@ -1235,7 +1243,7 @@ fn maybeStartDownload(self: *Frame, response: HttpClient.Response) !bool { } const disposition: HttpClient.Header = blk: { - var it = response.headerIterator(); + var it = transfer.responseHeaderIterator(); while (it.next()) |hdr| { if (std.ascii.eqlIgnoreCase(hdr.name, "content-disposition")) { break :blk hdr; @@ -1281,7 +1289,7 @@ fn maybeStartDownload(self: *Frame, response: HttpClient.Response) !bool { return false; }; - const total: ?u64 = if (response.contentLength()) |cl| cl else null; + const total: ?u64 = if (transfer.getContentLength()) |cl| cl else null; self._parse_state = .{ .download = .{ .guid = guid, @@ -1364,14 +1372,14 @@ fn isUtf16Encoding(charset: []const u8) bool { return std.mem.eql(u8, charset, "UTF-16LE") or std.mem.eql(u8, charset, "UTF-16BE"); } -fn frameDataCallback(response: HttpClient.Response, data: []const u8) !void { - var self: *Frame = @ptrCast(@alignCast(response.ctx)); +fn frameDataCallback(transfer: *HttpClient.Transfer, data: []const u8) !void { + var self: *Frame = @ptrCast(@alignCast(transfer.req.ctx)); if (self._parse_state == .pre) { // we lazily do this, because we might need the first chunk of data // to sniff the content type var mime: Mime = blk: { - if (response.contentType()) |ct| { + if (transfer.contentType()) |ct| { break :blk try Mime.parse(ct); } break :blk Mime.sniff(data); @@ -2096,18 +2104,10 @@ pub fn loadExternalStylesheet(self: *Frame, link: *Element.Html.Link, href: []co const http_client = &session.browser.http_client; - // `syncRequest` below registers a blocking request for this frame, which - // makes the DeferringLayer hold back the completion callbacks of every - // OTHER in-flight transfer for the frame (e.g. a `