Fetch + ReadableStream #972

mookums · 2025-08-25T15:07:04Z

This implements fetch in Zig, allowing us to also gain access to the ReadableStream API.

karlseguin · 2025-08-26T01:40:57Z

src/browser/fetch/Headers.zig

+
+pub fn _get(self: *const Headers, header: []const u8, page: *Page) !?[]const u8 {
+    const arena = page.arena;
+    const key = try std.ascii.allocLowerString(arena, header);


can use the call_arena here, as you don't need the key to outlive this function call.

karlseguin · 2025-08-26T01:41:52Z

src/browser/fetch/Headers.zig

+    const key = try std.ascii.allocLowerString(arena, header);
+
+    const value = (self.headers.getEntry(key) orelse return null).value_ptr.*;
+    return try arena.dupe(u8, value);


You don't need to dupe this value at all, the string will get copied by v8. you can return self.headers.get(key) directly.

karlseguin · 2025-08-27T12:22:54Z

src/browser/streams/ReadableStreamDefaultReader.zig

+    done: bool,
+
+    pub fn get_value(self: *const ReadableStreamReadResult, page: *Page) !?[]const u8 {
+        return if (self.value) |value| try page.arena.dupe(u8, value) else null;


Probably don't need to dupe this:

return self.value;

karlseguin · 2025-08-27T12:23:57Z

src/browser/streams/ReadableStreamDefaultReader.zig

+    switch (stream.state) {
+        .readable => {
+            if (stream.queue.items.len > 0) {
+                const data = self.stream.queue.orderedRemove(0);


This is O(N), right? I have no clue what these streams are used for, so we know how big the queues (a) typically are and (b) can grow to?

Yes it is. Will circle back on this, mainly was waiting for us to go to 0.15.1 because a lot of the FIFO stuff was removed. Would be nice if we had our own nice Queue impl (or maybe I just use the PriorityQueue and always return .eq 🤷)

@mookums Can you address this change now?

karlseguin · 2025-08-27T12:25:26Z

src/browser/streams/ReadableStreamDefaultReader.zig

+        },
+        .closed => |_| {
+            if (stream.queue.items.len > 0) {
+                const data = try page.arena.dupe(u8, self.stream.queue.orderedRemove(0));


If the value is dupe'd using the page.arena when being inserted, I don't think you'll ever need to dupe it again (certainly not with the same arena).

krichprollsch · 2025-08-27T12:43:48Z

I tried to fetch http://127.0.0.1:1234/campfire-commerce/ and I get a response.json is not a function error.
We will have to implement https://developer.mozilla.org/en-US/docs/Web/API/Response/json

src/browser/fetch/Headers.zig

krichprollsch · 2025-09-05T09:32:05Z

I'm testing the branch and comparing results w/ main.
It works quite well 👏

I found a difference fetching https://fr.pinterest.com/
I have the following error on this branch I don't have with main:

INFO  js : function call error . . . . . . . . . . . . . . .  [+1095ms]
      name = browser.html.window.Window._fetch
      err = InvalidArgument
      args =
        1: https://fr.pinterest.com/_/_/storage_report/ (string)
        2: {"method":"POST","headers":{"Content-Type":"application/json"},"body":"{\"storage-report\":{\"get\":[\"theme\"],\"set\":[],\"remove\":[]}}"} (object)
      stack =
        fetch:1
        <anonymous>:1
        d:18
        <anonymous>:18
        <anonymous>:18
        d:1
        a:1
        <anonymous>:1
        Promise:1
        <anonymous>:1
        v:1
        <anonymous>:1
        r:1
        <anonymous>:1
        d:18
        <anonymous>:18
        <anonymous>:18
        d:1
        a:1
        <anonymous>:1
        Promise:1
        <anonymous>:1
        b:1
        w:1
        value:1
        p:1
        237193:1
        n:1
        264788:1
        n:1

krichprollsch · 2025-09-05T09:38:50Z

src/browser/fetch/Request.zig

+        self.body,
+        .{},
+    ) catch |e| {
+        log.warn(.browser, "invalid json", .{ .err = e, .source = "Request" });


You should create and use a .fetch domain for all fetch's logs

This should be a debug or info. It's almost certainly a valid JS error that we can't do anything about. warn+ should be for things we can action.

krichprollsch · 2025-09-05T11:10:23Z

Same with https://jobs.lightpanda.io/

INFO  js : function call error . . . . . . . . . . . . . . .  [+799ms]
      name = browser.html.window.Window._fetch
      err = InvalidArgument
      args =
        1: /api/v3/getStatsigResults (string)
        2: {"method":"POST","credentials":"same-origin","headers":{"notion-client-version":"23.13.0.4890","notion-audit-log-platform":"web","x-notion-active-user-header":"","Content-Type":"application/json"},"body":"{\"deviceId\":\"9928347c-20cc-4366-aa1f-c00d3080f870
\",\"device\":{\"clientVersion\":\"23.13.0.4890\",\"isElectron\":false,\"isMobileNative\":false,\"isMobileBeta\":false,\"isMobileBrowser\":false,\"isBrowser\":true,\"isMobile\":false,\"isDesktopBrowser\":true,\"isTablet\":false,\"os\":\"unknown\",\"browserName\":\"und
efined\"},\"stableID\":\"d9ec7133-17de-4ba0-a206-c8ad22c49127\",\"locale\":\"en-US\",\"shouldStringifyInitialValues\":true,\"clientSdkApiKey\":\"client-Tgza5wNFa8dVt9BdeUfG6Vkm29bHxX10MhoztTMzLBB\"}"} (object)
      stack =
        m:54
        <anonymous>:54

mookums · 2025-09-08T06:49:17Z

Both of those resolved with 4413f68.

src/browser/fetch/Headers.zig

karlseguin · 2025-09-08T09:59:09Z

src/browser/fetch/Request.zig

+        self.body,
+        .{},
+    ) catch |e| {
+        log.warn(.browser, "invalid json", .{ .err = e, .source = "Request" });


This should be a debug or info. It's almost certainly a valid JS error that we can't do anything about. warn+ should be for things we can action.

src/browser/fetch/Request.zig

src/browser/fetch/Response.zig

mookums · 2025-09-08T14:56:11Z

Ready for review by whoever, just don't merge yet as I'm going to migrate the tests to the new htmlRunner.

krichprollsch · 2025-09-09T16:39:01Z

We have a hug memory regression:

Peak resident set size: 50120
https://github.com/lightpanda-io/browser/actions/runs/17554979612/job/49962587005?pr=972

mookums · 2025-09-09T22:01:05Z

We have a hug memory regression:

Peak resident set size: 50120
https://github.com/lightpanda-io/browser/actions/runs/17554979612/job/49962587005?pr=972

Fixed I believe, was a Persistent PromiseResolver not being deinitalized.

src/browser/fetch/fetch.zig

karlseguin · 2025-09-16T04:49:24Z

src/browser/fetch/fetch.zig

+
+    const resolver = Env.PromiseResolver{
+        .js_context = page.main_context,
+        .resolver = v8.PromiseResolver.init(page.main_context.v8_context),


No other part of the WebAPI that needs to dig this deep into the JS implementation. The fetch code does this 10 times. Surely there's a way to clean this up and make it easier and less v8-specific to create promises

I changed it so all instances of this pattern now use the Env.PersistentPromiseResolver which should make it less v8 specific to use these.

karlseguin · 2025-09-16T04:59:27Z

src/browser/fetch/fetch.zig

+    };
+
+    // Add destructor callback for FetchContext.
+    try page.main_context.destructor_callbacks.append(arena, Env.DestructorCallback.init(fetch_ctx));


This isn't ideal. Really reaching into an implementation details. Do we really need to reject the promise on page shutdown?

The other issue, that we talked about, is that if you don't cancel the transfer, then when the page does it, the errorCallback is called, and at that point, the context is aborted. But I think we can make this safer. (Although, I notice now that the errorCallback doesn't seem to be doing anything)

Changed this so it only rejects the promise if we have an error that isn't error.Abort which is what we throw when the page is closed (I think). This allows us to just ditch using the destructor callbacks entirely.

src/browser/fetch/fetch.zig

This defaults to Auto, which means it runs when the call stack reaches 0. It appears that both Node and Deno set this to explicit. I don't really understand why Auto doesn't work. It says the call stack is the C++/C callstack, and I don't see what would block the current code from reaching a depth of 0. Still, we already have explicit calls to performMicrotasksCheckpoint which ties it holistically with our scheduler, so having it be explicit like this should...well make it more explicit This broke a test, but since the tests are being redone in the [fetch PR](#972) I simply removed the offending one.

karlseguin reviewed Aug 26, 2025

View reviewed changes

karlseguin reviewed Aug 27, 2025

View reviewed changes

mookums force-pushed the fetch branch from 1814076 to 504026f Compare September 3, 2025 00:14

karlseguin reviewed Sep 3, 2025

View reviewed changes

mookums force-pushed the fetch branch 3 times, most recently from 8c8ba1e to 47aa966 Compare September 5, 2025 05:24

krichprollsch reviewed Sep 5, 2025

View reviewed changes

mookums force-pushed the fetch branch from 340b8a2 to d19dc8c Compare September 8, 2025 06:38

karlseguin reviewed Sep 8, 2025

View reviewed changes

mookums marked this pull request as ready for review September 8, 2025 14:56

karlseguin reviewed Sep 15, 2025

View reviewed changes

src/browser/fetch/fetch.zig Outdated Show resolved Hide resolved

karlseguin reviewed Sep 16, 2025

View reviewed changes

karlseguin mentioned this pull request Sep 16, 2025

Set Isolate Microtask to Explicit #1051

Merged

mookums force-pushed the fetch branch 3 times, most recently from bd4dd38 to 2a7a8bc Compare September 16, 2025 18:36

mookums added 5 commits September 17, 2025 08:40

remove polyfill and add req/resp

56c6e8b

initial fetch in zig

df0b6d5

request url as null terminated

f9014bb

fetch callback logging

dc2addb

proper fetch method and body setting

a133a71

mookums and others added 24 commits September 17, 2025 08:43

working Header iterators

51ee313

implement remaining ReadableStream functionality

463440b

jsValueToZig for fixed sized arrays

f68f184

remove length check of fixed size

cf8f76b

avoid explicit memcpy

a7848f4

retain value, avoid str alloc

7766892

add logging on fetch error callback

2659043

support object as HeadersInit

9251180

use destructor callback for FetchContext

20463a6

headers iterators should not allocate

9bbd06c

simplify Headers

618fff0

use call arena for json in Req/Resp

bb2595e

simplify cloning of Req/Resp

fe89c2f

deinit persistent promise resolver

af75ce7

properly handle closed for ReadableStream

76dae43

migrate fetch tests to htmlRunner

969bfb4

copy our Request headers into the HTTP client

a875ce4

use content length to reserve body size

ed11eab

fix arena, add fetch test

cd763a7

remove meaningless text from test

24330a7

use Env.PersistentPromiseResolver

c553a2c

htmlRunner for ReadableStream tests, fix ReadableStream enqueue

2a0964f

stop using destructor callback for fetch

2a969f9

use fetch logging scope, clean some comments

f22ee54

mookums force-pushed the fetch branch from 4d1e416 to f22ee54 Compare September 17, 2025 15:48

PeristentPromiseResolver with page lifetime

f6f0e14

krichprollsch approved these changes Sep 18, 2025

View reviewed changes

krichprollsch merged commit 2d24e3c into main Sep 18, 2025
10 checks passed

krichprollsch deleted the fetch branch September 18, 2025 07:29

github-actions bot locked and limited conversation to collaborators Sep 18, 2025

Uh oh!

Fetch + ReadableStream #972

Fetch + ReadableStream #972

Uh oh!

Conversation

mookums commented Aug 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krichprollsch commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

krichprollsch commented Sep 5, 2025

Uh oh!

krichprollsch Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krichprollsch commented Sep 5, 2025

Uh oh!

mookums commented Sep 8, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mookums commented Sep 8, 2025

Uh oh!

krichprollsch commented Sep 9, 2025

Uh oh!

mookums commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

krichprollsch commented Aug 27, 2025 •

edited

Loading

krichprollsch Sep 5, 2025 •

edited

Loading

mookums commented Sep 9, 2025 •

edited

Loading