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

ReadbleStream reads to completion in server, does not stream #1886

Closed
guest271314 opened this issue Jan 24, 2023 · 21 comments · Fixed by #4251
Closed

ReadbleStream reads to completion in server, does not stream #1886

guest271314 opened this issue Jan 24, 2023 · 21 comments · Fixed by #4251
Labels
bug Something isn't working

Comments

@guest271314
Copy link

What version of Bun is running?

0.5.1

What platform is your computer?

x86_64 GNU/Linux

What steps can reproduce the bug?

bun_server.js

import { serve, file, } from 'bun';

serve({
  port: 8443,
  keyFile: './certificate.key',
  certFile: './certificate.pem',
  async fetch(req, server) {
    console.log(
      'fetch',
      req,
    );
    let arr = 'abcdefg'.split('');
    let body = new ReadableStream({
      async pull(c) {
        while (arr.length) {
          c.enqueue(arr.splice(0, 1)[0]);
          await new Promise((resolve) => setTimeout(resolve, 1000));
        } 
        c.close();
      }
    }); // TextEncoderStream() is undefined
    return new Response(
      body
      , {
          headers: {
            'Access-Control-Allow-Origin':'*',
            'Access-Control-Allow-Private-Network': 'true'
          }
        }
    )
  },
})
 ./bun run --no-install --hot bun_server.js

At console on Chromium 111

(await fetch('https://localhost:8443', {
  // duplex:'half'
})).body
  .pipeThrough(new TextDecoderStream())
  .pipeTo(new WritableStream({
  write(v) {
    console.log(v);
  }, 
  close() {
    console.log('Stream closed.');
  }
}));

What is the expected behavior?

a
b
c
d
e
f
g
Stream closed.

What do you see instead?

abcdefg
Stream closed.

Additional information

Using deno we get the expected result

const server = Deno.listenTls({
  port: 8443,
  certFile: 'certificate.pem',
  keyFile: 'certificate.key',
  alpnProtocols: ['h2', 'http/1.1'],
});

// Connections to the server will be yielded up as an async iterable.
for await (const conn of server) {
  // In order to not be blocking, we need to handle each connection individually
  // without awaiting the function
  serveHttp(conn);
}

async function serveHttp(conn) {
  // This "upgrades" a network connection into an HTTP connection.
  const httpConn = Deno.serveHttp(conn);
  // Each request sent over the HTTP connection will be yielded as an async
  // iterator from the HTTP connection.
  for await (const requestEvent of httpConn) {
    // The native HTTP server uses the web standard `Request` and `Response`
    // objects.
    let body = null;
    if (requestEvent.request.method === 'GET') {
      let arr = 'abcdefg'.split('');
      body = new ReadableStream({
        async pull(c) {
          while (arr.length) {
            c.enqueue(arr.splice(0, 1)[0]);
            await new Promise((resolve) => setTimeout(resolve, 100));
          }
          c.close();
        }
      }).pipeThrough(new TextEncoderStream());
    }
    // The requestEvent's `.respondWith()` method is how we send the response
    // back to the client.
    requestEvent.respondWith(
      new Response(body, {
        headers: {
          'Access-Control-Allow-Origin': '*',
          'Access-Control-Allow-Private-Network': 'true',
        },
      })
    );
  }
}
@guest271314 guest271314 added the bug Something isn't working label Jan 24, 2023
@Jarred-Sumner
Copy link
Collaborator

definitely a bug

It's supposed to send off the stream as soon as the async tick happens

In the meantime, could you try:

  1. set type to "direct" in ReadableStreamSource (object where pull is defined)
  2. instead of .enqueue, call .write

@guest271314

This comment was marked as off-topic.

@Jarred-Sumner
Copy link
Collaborator

What if you do this

let body = new ReadableStream({
      type: 'direct',
      async pull(c) {
        while (arr.length) {
          c.write(arr.splice(0, 1)[0]);
          c.flush();
          await new Promise((resolve) => setTimeout(resolve, 1000));
        } 
        c.close();
      });

@guest271314

This comment was marked as off-topic.

@guest271314

This comment was marked as off-topic.

@cirospaciari
Copy link
Member

Flush was fixed on #3073

@guest271314

This comment was marked as off-topic.

@guest271314

This comment was marked as off-topic.

@Jarred-Sumner Jarred-Sumner reopened this May 27, 2023
@Jarred-Sumner
Copy link
Collaborator

Jarred-Sumner commented May 27, 2023

Confrming that this is not fixed, curl reported no output until the final chunk was received.
image

image

@Jarred-Sumner
Copy link
Collaborator

Jarred-Sumner commented May 27, 2023

It is fixed with "type": "direct", however.

import { serve, file } from "bun";

serve({
  port: 8443,
  async fetch(req, server) {
    console.log("fetch", req);
    var remain = 10;
    let body = new ReadableStream({
      type: "direct",
      async pull(c) {
        while (remain--) {
          c.write(new Date().toTimeString() + "\n");
          c.flush();
          await new Promise((resolve) => setTimeout(resolve, 1000));
        }

        c.close();
      },
    }); // TextEncoderStream() is undefined
    return new Response(body, {
      headers: {
        "Access-Control-Allow-Origin": "*",
        "Access-Control-Allow-Private-Network": "true",
      },
    });
  },
});

@guest271314

This comment was marked as off-topic.

@Jarred-Sumner
Copy link
Collaborator

It is fixed with "type": "direct", however.

No, it's not.

Can you paste the snippet with "type": "direct" that isn't working? The above prints timestamps one-by-one

@guest271314

This comment was marked as off-topic.

@guest271314

This comment was marked as off-topic.

@Jarred-Sumner
Copy link
Collaborator

are you on the canary build of bun? bun upgrade --canary

@guest271314

This comment was marked as off-topic.

@guest271314

This comment was marked as off-topic.

@guest271314

This comment was marked as off-topic.

@unkhz
Copy link

unkhz commented Jun 30, 2023

Can confirm that the issue still exists. No change in behaviour with type: 'direct' or without it. Nothing is received from the stream before controller.close is called. After first request, "Welcome to Bun! To get started, return a Response object." is received instead of the streamed response.

$ bun upgrade --canary
Congrats! You're already on the latest canary build of bun
$ bun --version
0.6.12

@unkhz
Copy link

unkhz commented Jun 30, 2023

Adding the "\n" into the output seems to make it work, though, but only with type: 'direct'. 🤔

This does not work:

let i = 0
serve({
  port: SERVER_PORT,
  fetch(req) {
    let body = new ReadableStream({
      type: 'direct',
      async pull(controller) {
        while (i++ < 10) {
          controller.write(i.toString()) // <------- this line
          controller.flush()
          await wait(1000)
        }
        controller.close()
      },
    })
    return new Response(body, {
      headers: {
        'Access-Control-Allow-Origin': '*',
        'Access-Control-Allow-Private-Network': 'true',
      },
    })
  },
})

This works as expected:

let i = 0
serve({
  port: SERVER_PORT,
  fetch(req) {
    let body = new ReadableStream({
      type: 'direct',
      async pull(controller) {
        while (i++ < 10) {
          controller.write(i.toString() + '\n') // <------- this line
          controller.flush()
          await wait(1000)
        }
        controller.close()
      },
    })
    return new Response(body, {
      headers: {
        'Access-Control-Allow-Origin': '*',
        'Access-Control-Allow-Private-Network': 'true',
      },
    })
  },
})

@guest271314

This comment was marked as off-topic.

Jarred-Sumner added a commit that referenced this issue Aug 23, 2023
Jarred-Sumner added a commit that referenced this issue Aug 23, 2023
* Update WebKit

* Don't do async hooks things when async hooks are not enabled

* Smarter scheduling of event loop tasks with the http server

* less exciting approach

* Bump WebKit

* Another approach

* Fix body-stream tests

* Fixes #1886

* Fix UAF in fetch body streaming

* Missing from commit

* Fix leak

* Fix the other leak

* Fix test

* Fix crash

* missing duperef

* Make this code clearer

* Ignore empty chunks

* Fixes #3969

* Delete flaky test

* Update bun-linux-build.yml

* Fix memory issue

* fix result body, and .done status before the last callback, dont touch headers after sent once

* refactor HTTPClientResult

* less flasky corrupted test

* oops

* fix mutex invalid state

* fix onProgressUpdate deinit/unlock

* fix onProgressUpdate deinit/unlock

* oops

* remove verbose

* fix posible null use

* avoid http null

* metadata can still be used onReject after toResponse

* dont leak task.http

* fix flask tests

* less flask close tests

---------

Co-authored-by: Jarred Sumner <709451+Jarred-Sumner@users.noreply.github.com>
Co-authored-by: cirospaciari <ciro.spaciari@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants