oven-sh / bun

Incredibly fast JavaScript runtime, bundler, test runner, and package manager – all in one
https://bun.sh
Other
73.16k stars 2.68k forks source link

Tests involving chunked-encoding client fails #4139

Open uNetworkingAB opened 1 year ago

uNetworkingAB commented 1 year ago

What version of Bun is running?

0.7.3

What platform is your computer?

Linux

What steps can reproduce the bug?

The test at https://github.com/uNetworking/uWebSockets/blob/master/tests/smoke.mjs runs against a simple Crc32 computing HTTP server.

It takes data via HTTP POST either in chunked-encoding or fixed length body.

Bun acts as the client (but Deno can run the test as well) and sends data in various chunk sizes and total sizes.

Bun fails due to (if I remember correctly) lacking the proper Transfer-Encoding: chunked header, and never completes the test. Deno completes the test without any issues.

This is a long standing bug, since early 0.4.0 days and had not been fixed.

What is the expected behavior?

The test runs as it would with Deno, here is an example of a completed test: https://github.com/uNetworking/uWebSockets/actions/runs/5757781321/job/15609340345 (see step "Run smoke test")

What do you see instead?

Making request with body size: 0
Making chunked request with body size: 0
Making request with body size: 0
Making chunked request with body size: 0
Making request with body size: 32
Making chunked request with body size: 32
43 |     /* Make a crc32 comparison of the two (mind the newline in one!) */
44 |     const got = new TextDecoder().decode(body.value);
45 | 
46 |     const want = crc32(array).toString(16);
47 |     if (got.toString().slice(0, -1) !== want.toString()) {
48 |         throw new Error("failed chunked test");
                  ^
error: failed chunked test
      at /home/alexhultman/uWebSockets.js/uWebSockets/tests/smoke.mjs:48:14

Additional information

No response

Jarred-Sumner commented 1 year ago

This is a bug in the client, right? Not in the server?

If yes, that's due to lack of fetch() request body streaming support, which will be fixed in #4127

uNetworkingAB commented 1 year ago

Yes Bun/Deno only acts as the client in this test. The server is always the Crc32 example https://github.com/uNetworking/uWebSockets/blob/master/examples/Crc32.cpp

uNetworkingAB commented 1 year ago

FYI, tested in v0.8.0 and same issue

cirospaciari commented 1 year ago

on v0.8.0 the fixedCrc32Test passes and streams the result, but the test chunkedCrc32Test is the problem, we have some issues when sending the stream body as ReadableStream we will work on that.

uNetworkingAB commented 1 year ago

Yeah that makes sense. I see that it sets header Content-Length=0 when it should instead set Transfer-Encoding=chunked for the case of chunked

cirospaciari commented 1 year ago

Yeah basically we are never sending chunks, today we lack streaming in this direction, we are trying to get the body (as a blob returning empty in this case) and send it with Content-Length.

Related code https://github.com/oven-sh/bun/blob/9c68abdb8d51951f83b4b253cf5dd3922c2c58b5/src/bun.js/webcore/response.zig#L1543-L1548

Server logs (first one is without ReadableStream):

content-type: text/plain
connection: keep-alive
user-agent: Bun/0.8.0_debug
accept: */*
host: localhost:3000
accept-encoding: gzip, deflate
content-length: 32
 --- / --- 
content-type: application/octet-stream
connection: keep-alive
user-agent: Bun/0.8.0_debug
accept: */*
host: localhost:3000
accept-encoding: gzip, deflate
content-length: 0
uNetworkingAB commented 1 year ago

Just adding that 1.0 also has the issue. Congratulations to the release, btw 👍

uNetworkingAB commented 9 months ago

This is still failing the same way in 1.0.14