Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests involving chunked-encoding client fails #4139

Open
uNetworkingAB opened this issue Aug 13, 2023 · 8 comments
Open

Tests involving chunked-encoding client fails #4139

uNetworkingAB opened this issue Aug 13, 2023 · 8 comments
Labels
needs tests Something that needs more testing

Comments

@uNetworkingAB
Copy link

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

@uNetworkingAB uNetworkingAB added the bug Something isn't working label Aug 13, 2023
@Jarred-Sumner
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Author

FYI, tested in v0.8.0 and same issue

@cirospaciari
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

cirospaciari commented Aug 24, 2023

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

if (options.fastGet(ctx.ptr(), .body)) |body__| {
if (Body.Value.fromJS(ctx.ptr(), body__)) |body_const| {
var body_value = body_const;
// TODO: buffer ReadableStream?
// we have to explicitly check for InternalBlob
body = body_value.useAsAnyBlob();

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
Copy link
Author

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

@Electroid Electroid added needs tests Something that needs more testing and removed bug Something isn't working labels Oct 26, 2023
@uNetworkingAB
Copy link
Author

This is still failing the same way in 1.0.14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Something that needs more testing
Projects
None yet
Development

No branches or pull requests

4 participants