fix(ext/http): ensure request body resource lives as long as response is alive #20206

mmastrac · 2023-08-18T19:12:52Z

Deno.serve's fast streaming implementation was not keeping the request body resource ID alive. We were taking the Rc<Resource> from the resource table during the response, so a hairpin duplex response that fed back the request body would work.

However, if any JS code attempted to read from the request body (which requires the resource ID to be valid), the response would fail with a difficult-to-diagnose "EOF" error.

This was affecting more complex duplex uses of Deno.fetch (though as far as I can tell was unreported).

Simple test:

        const reader = request.body.getReader();
        return new Response(
          new ReadableStream({
            async pull(controller) {
              const { done, value } = await reader.read();
              if (done) {
                controller.close();
              } else {
                controller.enqueue(value);
              }
            },
          }),

And then attempt to use the stream in duplex mode:

async function testDuplex(
  reader: ReadableStreamDefaultReader<Uint8Array>,
  writable: WritableStreamDefaultWriter<Uint8Array>,
) {
  await writable.write(new Uint8Array([1]));
  const chunk1 = await reader.read();
  assert(!chunk1.done);
  assertEquals(chunk1.value, new Uint8Array([1]));
  await writable.write(new Uint8Array([2]));
  const chunk2 = await reader.read();
  assert(!chunk2.done);
  assertEquals(chunk2.value, new Uint8Array([2]));
  await writable.close();
  const chunk3 = await reader.read();
  assert(chunk3.done);
}

In older versions of Deno, this would just lock up. I believe after 23ff0e7, it started throwing a more explicit error:

httpServerStreamDuplexJavascript => ./cli/tests/unit/serve_test.ts:1339:6
error: TypeError: request or response body error: error reading a body from connection: Connection reset by peer (os error 54)
    at async Object.pull (ext:deno_web/06_streams.js:810:27)

mmastrac · 2023-08-19T15:28:06Z

cli/tests/unit/serve_test.ts

@@ -721,7 +721,7 @@ function createStreamTest(count: number, delay: number, action: string) {
 }

 for (const count of [0, 1, 2, 3]) {
-  for (const delay of [0, 1, 1000]) {
+  for (const delay of [0, 1, 25]) {


We no longer need a 1000ms test here -- there are no more timeouts in Deno.serve that exercise this path, so we can just use 25ms and make tests faster.

mmastrac · 2023-08-19T15:31:41Z

ext/http/00_serve.js

@@ -133,10 +133,6 @@ class InnerRequest {
  }

  close() {
-    if (this.#streamRid !== undefined) {


Stream closing now happens in Rust.

bartlomieju

LGTM

ext/http/http_next.rs

ext/http/response_body.rs

… is alive (denoland#20206) Deno.serve's fast streaming implementation was not keeping the request body resource ID alive. We were taking the `Rc<Resource>` from the resource table during the response, so a hairpin duplex response that fed back the request body would work. However, if any JS code attempted to read from the request body (which requires the resource ID to be valid), the response would fail with a difficult-to-diagnose "EOF" error. This was affecting more complex duplex uses of `Deno.fetch` (though as far as I can tell was unreported). Simple test: ```ts const reader = request.body.getReader(); return new Response( new ReadableStream({ async pull(controller) { const { done, value } = await reader.read(); if (done) { controller.close(); } else { controller.enqueue(value); } }, }), ``` And then attempt to use the stream in duplex mode: ```ts async function testDuplex( reader: ReadableStreamDefaultReader<Uint8Array>, writable: WritableStreamDefaultWriter<Uint8Array>, ) { await writable.write(new Uint8Array([1])); const chunk1 = await reader.read(); assert(!chunk1.done); assertEquals(chunk1.value, new Uint8Array([1])); await writable.write(new Uint8Array([2])); const chunk2 = await reader.read(); assert(!chunk2.done); assertEquals(chunk2.value, new Uint8Array([2])); await writable.close(); const chunk3 = await reader.read(); assert(chunk3.done); } ``` In older versions of Deno, this would just lock up. I believe after 23ff0e7, it started throwing a more explicit error: ``` httpServerStreamDuplexJavascript => ./cli/tests/unit/serve_test.ts:1339:6 error: TypeError: request or response body error: error reading a body from connection: Connection reset by peer (os error 54) at async Object.pull (ext:deno_web/06_streams.js:810:27) ```

… is alive (#20206) Deno.serve's fast streaming implementation was not keeping the request body resource ID alive. We were taking the `Rc<Resource>` from the resource table during the response, so a hairpin duplex response that fed back the request body would work. However, if any JS code attempted to read from the request body (which requires the resource ID to be valid), the response would fail with a difficult-to-diagnose "EOF" error. This was affecting more complex duplex uses of `Deno.fetch` (though as far as I can tell was unreported). Simple test: ```ts const reader = request.body.getReader(); return new Response( new ReadableStream({ async pull(controller) { const { done, value } = await reader.read(); if (done) { controller.close(); } else { controller.enqueue(value); } }, }), ``` And then attempt to use the stream in duplex mode: ```ts async function testDuplex( reader: ReadableStreamDefaultReader<Uint8Array>, writable: WritableStreamDefaultWriter<Uint8Array>, ) { await writable.write(new Uint8Array([1])); const chunk1 = await reader.read(); assert(!chunk1.done); assertEquals(chunk1.value, new Uint8Array([1])); await writable.write(new Uint8Array([2])); const chunk2 = await reader.read(); assert(!chunk2.done); assertEquals(chunk2.value, new Uint8Array([2])); await writable.close(); const chunk3 = await reader.read(); assert(chunk3.done); } ``` In older versions of Deno, this would just lock up. I believe after 23ff0e7, it started throwing a more explicit error: ``` httpServerStreamDuplexJavascript => ./cli/tests/unit/serve_test.ts:1339:6 error: TypeError: request or response body error: error reading a body from connection: Connection reset by peer (os error 54) at async Object.pull (ext:deno_web/06_streams.js:810:27) ```

mcandeia · 2023-08-23T14:56:39Z

Hello guys, I am facing a problem that only happens on deno 1.36.2, I was taking a look in the repository trying to figure which could be the change that potentially caused this and due to my lack of knowledge in rust I selected this one cause the title is close enough to what I've done to fix the issue so far.

The problem is that the transfer-encoding was changed to chunked (previously no transfer-encoding was set by deno) when a new Response(body, {headers, status}) is used using a previously created response as a parameter. Looks like our front-end framework (fresh, currently) is not handling well the idea of receiving this data chunked.

Check my PR changes to make it work if it could help: https://github.com/deco-cx/deco/pull/392/files

Tested with + deno 1.36.1

Deno.serve((_req) => {
  const resp = new Response("hello, world");
  return new Response(resp.body);
});

Then

curl http://localhost:8000 -vvv

Now with deno 1.36.2 (used deno upgrade 1.36.2)

cc: @mmastrac @bartlomieju

mcandeia · 2023-08-23T16:09:22Z

tbh can be this one too: https://github.com/denoland/deno/pull/20057/files

mmastrac · 2023-08-23T16:25:11Z

I think it might be from #20180

@mcandeia Can you explain what's going on in Fresh? Chunked encoding vs content-length is an internal detail that we only offer a best effort, unless you specifically pass a buffer or string.

What is specifically failing in fresh?

mcandeia · 2023-08-23T17:02:49Z

What is specifically failing in fresh?

Looks like islands larger than size X are broken, partially streamed to the browser. same for large HTMLs. I'm trying to reproduce it in a simple example than I can post it here.

cc: @marvinhagemeister

mcandeia · 2023-08-23T17:07:26Z

Let's move the discussion to the right issue then? I can reproduce this using fresh getting started, I'm going to create a sample repo

mcandeia · 2023-08-23T17:15:59Z

Issue created on fresh: #20252

mmastrac · 2023-08-23T17:35:03Z

@mcandeia Thanks for the repro! I'll look at this ASAP.

mmastrac force-pushed the close_resource branch from 073f313 to 36c3a6e Compare August 18, 2023 19:36

mmastrac commented Aug 19, 2023

View reviewed changes

bartlomieju approved these changes Aug 19, 2023

View reviewed changes

ext/http/http_next.rs Show resolved Hide resolved

ext/http/response_body.rs Outdated Show resolved Hide resolved

mmastrac force-pushed the close_resource branch from 4f2eb22 to e3e94fa Compare August 20, 2023 16:07

mmastrac enabled auto-merge (squash) August 20, 2023 16:43

mmastrac force-pushed the close_resource branch from 0bf571a to 968e8f7 Compare August 20, 2023 17:33

mmastrac added 2 commits August 20, 2023 11:40

test

ec70fb8

fmt

0dd0ebf

mmastrac force-pushed the close_resource branch from 968e8f7 to 0dd0ebf Compare August 21, 2023 00:53

mmastrac merged commit 576d0db into denoland:main Aug 21, 2023
13 checks passed

lucab mentioned this pull request Aug 28, 2023

ext/http: panic on "already borrowed" (BorrowMutError) #20315

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ext/http): ensure request body resource lives as long as response is alive #20206

fix(ext/http): ensure request body resource lives as long as response is alive #20206

mmastrac commented Aug 18, 2023 •

edited

mmastrac Aug 19, 2023

mmastrac Aug 19, 2023

bartlomieju left a comment

mcandeia commented Aug 23, 2023

mcandeia commented Aug 23, 2023

mmastrac commented Aug 23, 2023 •

edited

mcandeia commented Aug 23, 2023

mcandeia commented Aug 23, 2023

mcandeia commented Aug 23, 2023

mmastrac commented Aug 23, 2023

fix(ext/http): ensure request body resource lives as long as response is alive #20206

fix(ext/http): ensure request body resource lives as long as response is alive #20206

Conversation

mmastrac commented Aug 18, 2023 • edited

mmastrac Aug 19, 2023

Choose a reason for hiding this comment

mmastrac Aug 19, 2023

Choose a reason for hiding this comment

bartlomieju left a comment

Choose a reason for hiding this comment

mcandeia commented Aug 23, 2023

mcandeia commented Aug 23, 2023

mmastrac commented Aug 23, 2023 • edited

mcandeia commented Aug 23, 2023

mcandeia commented Aug 23, 2023

mcandeia commented Aug 23, 2023

mmastrac commented Aug 23, 2023

mmastrac commented Aug 18, 2023 •

edited

mmastrac commented Aug 23, 2023 •

edited