http-server doesn't properly terminate BYOB reader

[vlovich]

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch @miniflare/[email protected] for the project I'm working on.

I noticed that I had done a readAtLeast(64kb, new Uint8Array(64kb)) and it hung on a body of 15 bytes. @jasnell had pointed out that the MDN docs don't document that if there's a pending read waiting, you have to controller.byobRequest?.respond(0) to have the ongoing read notice the end of the stream & unblock.

Here is the diff that solved my problem:

diff --git a/node_modules/@miniflare/http-server/dist/src/index.js b/node_modules/@miniflare/http-server/dist/src/index.js
index d7a4ebc..70ffbbc 100644
--- a/node_modules/@miniflare/http-server/dist/src/index.js
+++ b/node_modules/@miniflare/http-server/dist/src/index.js
@@ -404,7 +404,10 @@ async function convertNodeRequest(req, meta) {
       async pull(controller) {
         const { done, value } = await iterator.next();
         if (done) {
-          queueMicrotask(() => controller.close());
+          queueMicrotask(() => {
+              controller.close()
+              controller.byobRequest?.respond(0)
+          });
         } else {
           const buffer = Buffer.isBuffer(value) ? value : Buffer.from(value);
           controller.enqueue(new Uint8Array(buffer));

This issue body was partially generated by patch-package.

[vlovich]

The same problem appears in @miniflare/core:

diff --git a/node_modules/@miniflare/core/dist/src/index.js b/node_modules/@miniflare/core/dist/src/index.js
index 2909a6d..f4f25c9 100644
--- a/node_modules/@miniflare/core/dist/src/index.js
+++ b/node_modules/@miniflare/core/dist/src/index.js
@@ -529,8 +529,10 @@ var Body = class {
         } else if (value) {
           return controller.error(new TypeError(buildNotBufferSourceError(value)));
         }
-        if (done)
+        if (done) {
           controller.close();
+          controller.byobRequest?.respond(0);
+        }
       },
       cancel: (reason) => reader.cancel(reason)
     };

It may also be present in KV's convertStoredToGetValue but I'm not sure (same pattern of calling controller.close).

[mrbbot]

Hey! 👋 Thanks for raising this. This probably is a problem in KV too. Didn't know about controller.byobRequest?.respond(0). 😃

[mrbbot]

Hey! I've just released version 2.5.0 with this fix.