fetch(stream) add stream support for compressed and uncompressed data #4127

@@ -73,6 +73,8 @@ pub fn UnboundedQueue(comptime T: type, comptime next_field: meta.FieldEnum(T))
        }

        pub fn pop(self: *Self) ?*T {
+            if (self.isEmpty()) return null;


What happens if count is incremented after isEmpty() returns true?

Why isn't it sufficient for front to be null and then return?

I think I can revert this, this only evades the problem but is still race, for some reason we are getting an assertion error on assert(@atomicRmw(usize, &self.count, .Sub, 1, .Monotonic) >= 1);

Maybe we should use popBatch

src/bun.js/webcore/body.zig

src/bun.js/webcore/response.zig

Jarred-Sumner · 2023-08-19T05:27:44Z

src/bun.js/webcore/body.zig

        pub fn size(this: *const Value) Blob.SizeType {
            return switch (this.*) {
                .Blob => this.Blob.size,
                .InternalBlob => @as(Blob.SizeType, @truncate(this.InternalBlob.sliceConst().len)),
                .WTFStringImpl => @as(Blob.SizeType, @truncate(this.WTFStringImpl.utf8ByteLength())),
+                .Locked => {


Suggested change

.Locked => {

.Locked => this._lcokedSize()

done in 4676cfe

Jarred-Sumner · 2023-08-19T05:28:06Z

src/bun.js/webcore/response.zig

@@ -619,23 +621,32 @@ pub const Fetch = struct {
        javascript_vm: *VirtualMachine = undefined,
        global_this: *JSGlobalObject = undefined,
        request_body: HTTPRequestBody = undefined,
+        // buffer being used by AsyncHTTP


use /// because // doesn't show up in autocomplete

fixed 4676cfe

Jarred-Sumner · 2023-08-19T05:30:30Z

src/bun.js/webcore/response.zig

+                            return;
+                        }
+                    } else {
+                        this.response.?.body.value.Locked.size_hint = @as(u52, @intCast(this.body_size));


is the pointer aliasing here intentional?

fixed 4676cfe

Jarred-Sumner · 2023-08-19T05:32:22Z

src/bun.js/webcore/response.zig

+                return;
+            }
+
+            if (this.response) |response| {


What is keeping this response alive? How can we be sure that the garbage collector doesn't free the Response object in the middle of streaming?

fixed 4676cfe

Jarred-Sumner · 2023-08-19T05:33:27Z

I think this needs about two dozen more tests

Jarred-Sumner · 2023-08-20T01:12:56Z

looks like macOS build fails

Jarred-Sumner · 2023-08-22T01:16:33Z

src/bun.js/bindings/bindings.cpp

@@ -1181,7 +1181,7 @@ WebCore::FetchHeaders* WebCore__FetchHeaders__createFromPicoHeaders_(const void*

        for (size_t j = 0; j < end; j++) {
            PicoHTTPHeader header = pico_headers.ptr[j];
-            if (header.value.len == 0)
+            if (header.value.len == 0 || header.name.len == 0)


interesting

Jarred-Sumner · 2023-08-22T01:17:43Z

src/bun.js/webcore/blob.zig

@@ -3794,6 +3794,10 @@ pub const Blob = struct {
                        } else {
                            return build.blob.dupe();
                        }
+                    } else if (current.toSliceOrNull(global)) |sliced| {
+                        if (sliced.allocator.get()) |allocator| {
+                            return Blob.initWithAllASCII(bun.constStrToU8(sliced.slice()), allocator, global, false);


This should be cloned. There are too many cases where we assume Blob is always owned by mimalloc, but in this case it might not be.

also if you do .arrayBuffer() on a Response it will transfer the buffer which would mean that the string is now mutable which will break an unknown number of things.

weird toSliceOrNull dont clone it?
https://github.com/oven-sh/bun/blob/main/src/bun.js/bindings/bindings.zig#L4193-L4198

toSlice does not if it's an ASCII latin1 string. toOwnedSlice clones it.

fixed on ef6989f

Jarred-Sumner · 2023-08-22T01:19:16Z

src/bun.js/webcore/body.zig

                // .InlineBlob => @truncate(Blob.SizeType, this.InlineBlob.sliceConst().len),
                else => 0,
            };
        }

        pub fn estimatedSize(this: *const Value) usize {
            return switch (this.*) {
+                .Blob => this.Blob.size,


This is not correct. Blob.size defaults to a max value which is an absurdly large number to indicate that it is not a valid number. This would need to call resolveSize which would be too expensive to call here because it potentially does stat() on a file in the filesystem.

fixed on ef6989f

Jarred-Sumner · 2023-08-22T01:20:28Z

src/bun.js/webcore/response.zig

+        }
+
+        fn getSizeHint(this: *FetchTasklet) u52 {
+            if (this.body_size == .content_length) {


it would be more idiomatic Zig for this to be a switch statement

fixed on ef6989f

Jarred-Sumner · 2023-08-22T01:20:47Z

src/bun.js/webcore/response.zig

            };
+        }
+
+        fn getSizeHint(this: *FetchTasklet) u52 {


Can you use Blob.SizeType instead of directly using u52? I think that makes it more clear that it is consistent

fixed on ef6989f

Jarred-Sumner · 2023-08-22T01:20:59Z

src/bun.js/webcore/response.zig

+
+        fn getSizeHint(this: *FetchTasklet) u52 {
+            if (this.body_size == .content_length) {
+                return @as(u52, @intCast(this.body_size.content_length));


This should be truncate and the @as is unnecessary

fixed on ef6989f

Jarred-Sumner · 2023-08-22T01:21:15Z

src/bun.js/webcore/response.zig

@@ -830,7 +1022,10 @@ pub const Fetch = struct {
            const allocator = bun.default_allocator;
            var response = allocator.create(Response) catch unreachable;
            response.* = this.toResponse(allocator);
-            return Response.makeMaybePooled(@as(js.JSContextRef, @ptrCast(this.global_this)), response);
+            const response_js = Response.makeMaybePooled(@as(js.JSContextRef, @ptrCast(this.global_this)), response);


the @ptrCast here does nothing btw

fixed on ef6989f

Jarred-Sumner · 2023-08-22T01:22:02Z

src/bun.js/webcore/streams.zig

-                        .err = Syscall.Error{
-                            .errno = @as(Syscall.Error.Int, @intCast(-completion.result)),
-                            .syscall = .read,
+                        .err = .{


err.err is kinda weird

yeah will rename to sys_err

Jarred-Sumner · 2023-08-22T01:27:37Z

test/js/web/fetch/fetch.stream.test.ts

+            }
+          }
+          gcTick(false);
+          expect(buffer.byteLength).toBe(content.byteLength);


This only checks the length. It needs to check the value too. expect(buffer).toEqual(content)

Will add it

fixed on 70d8e69

Jarred-Sumner · 2023-08-22T01:29:09Z

Thank you for writing the tests. I think we can ship this

Jarred-Sumner

any idea why the fsevents test is failing?

Jarred-Sumner · 2023-08-22T01:30:47Z

src/bun.js/event_loop.zig

@@ -545,8 +545,7 @@ pub const EventLoop = struct {
                },
                .FetchTasklet => {
                    var fetch_task: *Fetch.FetchTasklet = task.get(Fetch.FetchTasklet).?;
-                    fetch_task.onDone();
-                    fetch_task.deinit();
+                    fetch_task.onProgressUpdate();


when does the task itself get freed?

it self deinit after receiving the has_more == false after clearData calls

but the Tasklet part. The thing which is queued to the tasks queue.

but the Tasklet part. The thing which is queued to the tasks queue.

Oh I see dummy mistake, let me fix it ASAP

Actually only one task is allocated, and on the last callback it is freed. We use task.javascript_vm.eventLoop().enqueueTaskConcurrent(task.concurrent_task.from(task, .manual_deinit)); so we use the same pointer to the task, not need to be freed, on each callback only in the last one the deinits are:

bun/src/bun.js/webcore/response.zig

Line 735 in ef6989f

this.deinit();

bun/src/bun.js/webcore/response.zig

Line 855 in ef6989f

this.deinit();

bun/src/bun.js/webcore/response.zig

Line 863 in ef6989f

this.deinit();

cirospaciari · 2023-08-22T01:30:50Z

any idea why the fsevents test is failing?

no idea, this poped up after the last rebase

Jarred-Sumner · 2023-08-22T01:32:04Z

src/bun.js/webcore/streams.zig

@@ -525,9 +525,19 @@ pub const StreamResult = union(Tag) {
    into_array: IntoArray,
    into_array_and_done: IntoArray,
    pending: *Pending,
-    err: Syscall.Error,
+
+    err: union(Err) {


maybe do

Error: Syscall.Error JSValue: JSC.JSValue

That JSvalue version also needs to be kept alive somehow.

Fixed on 4d07c98

Jarred-Sumner · 2023-08-22T03:30:59Z

🥳 🥳 🥳

Jarred-Sumner reviewed Aug 12, 2023

View reviewed changes

misctools/fetch.zig Outdated Show resolved Hide resolved

Jarred-Sumner reviewed Aug 12, 2023

View reviewed changes

src/deps/uws.zig Outdated Show resolved Hide resolved

This was referenced Aug 12, 2023

fetch doesn't stream #4132

Closed

Tests involving chunked-encoding client fails #4139

Open

cirospaciari force-pushed the ciro/fetch-streaming branch from d7f6800 to bad4db1 Compare August 16, 2023 15:51

cirospaciari changed the title ~~WIP: fetch(stream) streams non compressed data in 64kb chunks (at least)~~ WIP: fetch(stream) add stream support for compressed and uncompressed data Aug 17, 2023

cirospaciari marked this pull request as ready for review August 17, 2023 23:33

cirospaciari force-pushed the ciro/fetch-streaming branch from 6a4e1cb to 6189205 Compare August 17, 2023 23:50

Jarred-Sumner marked this pull request as draft August 18, 2023 02:54

cirospaciari force-pushed the ciro/fetch-streaming branch 2 times, most recently from 0757809 to 86f0dd1 Compare August 19, 2023 01:26

cirospaciari changed the title ~~WIP: fetch(stream) add stream support for compressed and uncompressed data~~ fetch(stream) add stream support for compressed and uncompressed data Aug 19, 2023

cirospaciari requested a review from Jarred-Sumner August 19, 2023 02:36

cirospaciari marked this pull request as ready for review August 19, 2023 02:37

Jarred-Sumner reviewed Aug 19, 2023

View reviewed changes

src/bun.js/webcore/body.zig Outdated Show resolved Hide resolved

Jarred-Sumner reviewed Aug 19, 2023

View reviewed changes

src/bun.js/webcore/response.zig Outdated Show resolved Hide resolved

Jarred-Sumner reviewed Aug 19, 2023

View reviewed changes

cirospaciari force-pushed the ciro/fetch-streaming branch from aa5e79b to f42efc7 Compare August 19, 2023 19:04

cirospaciari added 5 commits August 21, 2023 20:45

ignore headers with no name, add more tests

66d785e

oops

1868fc1

handle transform with fetch

9c92809

add gz image stream test

c8f8c75

remove duplicate test

7cb1869

cirospaciari force-pushed the ciro/fetch-streaming branch from 1e474e8 to 7cb1869 Compare August 21, 2023 23:45

cirospaciari requested a review from Jarred-Sumner August 22, 2023 00:40

fix missing chunk on macOS under pressure

a54860c

Jarred-Sumner reviewed Aug 22, 2023

View reviewed changes

oops include all OS

4e37033

Jarred-Sumner reviewed Aug 22, 2023

View reviewed changes

Jarred-Sumner approved these changes Aug 22, 2023

View reviewed changes

Jarred-Sumner reviewed Aug 22, 2023

View reviewed changes

cirospaciari added 3 commits August 21, 2023 22:13

some fixes

ef6989f

compare buffers instead of sizes

70d8e69

refactor err.err and protect it

4d07c98

Jarred-Sumner merged commit 9027484 into main Aug 22, 2023
15 of 20 checks passed

Jarred-Sumner deleted the ciro/fetch-streaming branch August 22, 2023 03:30

ambergristle mentioned this pull request Feb 4, 2024

Express post request with transfer-encoding chuncked has no body #8638

Open

fetch(stream) add stream support for compressed and uncompressed data #4127

fetch(stream) add stream support for compressed and uncompressed data #4127

Conversation

cirospaciari commented Aug 12, 2023 • edited Loading

What does this PR do?

How did you verify your code works?

github-actions bot commented Aug 12, 2023 • edited Loading

github-actions bot commented Aug 18, 2023 • edited Loading

github-actions bot commented Aug 18, 2023 • edited Loading

github-actions bot commented Aug 18, 2023 • edited Loading

github-actions bot commented Aug 18, 2023 • edited Loading

CodeFromAnywhere commented Aug 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jarred-Sumner commented Aug 19, 2023

Jarred-Sumner commented Aug 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jarred-Sumner commented Aug 22, 2023

Jarred-Sumner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cirospaciari Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cirospaciari commented Aug 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jarred-Sumner commented Aug 22, 2023

cirospaciari commented Aug 12, 2023 •

edited

Loading

github-actions bot commented Aug 12, 2023 •

edited

Loading

github-actions bot commented Aug 18, 2023 •

edited

Loading

github-actions bot commented Aug 18, 2023 •

edited

Loading

github-actions bot commented Aug 18, 2023 •

edited

Loading

github-actions bot commented Aug 18, 2023 •

edited

Loading

cirospaciari Aug 22, 2023 •

edited

Loading