From dd1b4327788016173c64ecb9b569791ffee3b473 Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Wed, 15 May 2024 17:06:55 -0400 Subject: [PATCH 1/7] Fix streaming in Node.js fast path --- .changeset/fluffy-pears-press.md | 5 +++++ .../src/runtime/server/render/astro/render.ts | 14 ++++++++------ 2 files changed, 13 insertions(+), 6 deletions(-) create mode 100644 .changeset/fluffy-pears-press.md diff --git a/.changeset/fluffy-pears-press.md b/.changeset/fluffy-pears-press.md new file mode 100644 index 000000000000..03eadc1347c9 --- /dev/null +++ b/.changeset/fluffy-pears-press.md @@ -0,0 +1,5 @@ +--- +"astro": patch +--- + +Fix streaming in Node.js fast path diff --git a/packages/astro/src/runtime/server/render/astro/render.ts b/packages/astro/src/runtime/server/render/astro/render.ts index f918f55c11dc..6134f6426e63 100644 --- a/packages/astro/src/runtime/server/render/astro/render.ts +++ b/packages/astro/src/runtime/server/render/astro/render.ts @@ -209,14 +209,17 @@ export async function renderToAsyncIterable( let error: Error | null = null; // The `next` is an object `{ promise, resolve, reject }` that we use to wait // for chunks to be pushed into the buffer. - let next = promiseWithResolvers(); + let next: ReturnType> | null = null; const buffer: Uint8Array[] = []; // []Uint8Array const iterator: AsyncIterator = { async next() { if (result.cancelled) return { done: true, value: undefined }; - await next.promise; + if(next !== null) { + await next.promise; + } + next = promiseWithResolvers(); // If an error occurs during rendering, throw the error as we cannot proceed. if (error) { @@ -276,8 +279,7 @@ export async function renderToAsyncIterable( // Push the chunks into the buffer and resolve the promise so that next() // will run. buffer.push(bytes); - next.resolve(); - next = promiseWithResolvers(); + next?.resolve(); } }, }; @@ -286,12 +288,12 @@ export async function renderToAsyncIterable( renderPromise .then(() => { // Once rendering is complete, calling resolve() allows the iterator to finish running. - next.resolve(); + next?.resolve(); }) .catch((err) => { // If an error occurs, save it in the scope so that we throw it when next() is called. error = err; - next.resolve(); + next?.resolve(); }); // This is the Iterator protocol, an object with a `Symbol.asyncIterator` From e272196bf67a0a95d3d51be8e32a82c6a20eb471 Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Wed, 15 May 2024 17:16:14 -0400 Subject: [PATCH 2/7] Create a new next if the iterator is not done --- .../astro/src/runtime/server/render/astro/render.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/astro/src/runtime/server/render/astro/render.ts b/packages/astro/src/runtime/server/render/astro/render.ts index 6134f6426e63..fbe54947d19b 100644 --- a/packages/astro/src/runtime/server/render/astro/render.ts +++ b/packages/astro/src/runtime/server/render/astro/render.ts @@ -219,7 +219,6 @@ export async function renderToAsyncIterable( if(next !== null) { await next.promise; } - next = promiseWithResolvers(); // If an error occurs during rendering, throw the error as we cannot proceed. if (error) { @@ -244,9 +243,14 @@ export async function renderToAsyncIterable( // Empty the array. We do this so that we can reuse the same array. buffer.length = 0; + // The iterator is done if there are no chunks to return. + let done = length === 0; + if(!done) { + next = promiseWithResolvers(); + } + const returnValue = { - // The iterator is done if there are no chunks to return. - done: length === 0, + done, value: mergedArray, }; From a6ff23de046342862081585c69d0b32e96943bf6 Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Thu, 16 May 2024 08:51:19 -0400 Subject: [PATCH 3/7] Use a flag instead --- .../src/runtime/server/render/astro/render.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/astro/src/runtime/server/render/astro/render.ts b/packages/astro/src/runtime/server/render/astro/render.ts index fbe54947d19b..7099ffa86f19 100644 --- a/packages/astro/src/runtime/server/render/astro/render.ts +++ b/packages/astro/src/runtime/server/render/astro/render.ts @@ -211,6 +211,7 @@ export async function renderToAsyncIterable( // for chunks to be pushed into the buffer. let next: ReturnType> | null = null; const buffer: Uint8Array[] = []; // []Uint8Array + let renderingComplete = false; const iterator: AsyncIterator = { async next() { @@ -218,6 +219,10 @@ export async function renderToAsyncIterable( if(next !== null) { await next.promise; + + if(!renderingComplete) { + next = promiseWithResolvers(); + } } // If an error occurs during rendering, throw the error as we cannot proceed. @@ -243,14 +248,9 @@ export async function renderToAsyncIterable( // Empty the array. We do this so that we can reuse the same array. buffer.length = 0; - // The iterator is done if there are no chunks to return. - let done = length === 0; - if(!done) { - next = promiseWithResolvers(); - } - const returnValue = { - done, + // The iterator is done if there are no chunks to return. + done: length === 0, value: mergedArray, }; @@ -292,11 +292,13 @@ export async function renderToAsyncIterable( renderPromise .then(() => { // Once rendering is complete, calling resolve() allows the iterator to finish running. + renderingComplete = true; next?.resolve(); }) .catch((err) => { // If an error occurs, save it in the scope so that we throw it when next() is called. error = err; + renderingComplete = true; next?.resolve(); }); From 08cb9a95fec2593d88a00421b1c725cb1706e1cc Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Thu, 16 May 2024 09:21:27 -0400 Subject: [PATCH 4/7] Update test --- packages/astro/src/runtime/server/render/astro/render.ts | 6 +++--- packages/astro/test/streaming.test.js | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/astro/src/runtime/server/render/astro/render.ts b/packages/astro/src/runtime/server/render/astro/render.ts index 7099ffa86f19..efed9751b82a 100644 --- a/packages/astro/src/runtime/server/render/astro/render.ts +++ b/packages/astro/src/runtime/server/render/astro/render.ts @@ -219,10 +219,10 @@ export async function renderToAsyncIterable( if(next !== null) { await next.promise; + } - if(!renderingComplete) { - next = promiseWithResolvers(); - } + if(!renderingComplete) { + next = promiseWithResolvers(); } // If an error occurs during rendering, throw the error as we cannot proceed. diff --git a/packages/astro/test/streaming.test.js b/packages/astro/test/streaming.test.js index 93172aa72a1d..cbd4fa4f4b46 100644 --- a/packages/astro/test/streaming.test.js +++ b/packages/astro/test/streaming.test.js @@ -37,7 +37,7 @@ describe('Streaming', () => { let chunk = decoder.decode(bytes); chunks.push(chunk); } - assert.equal(chunks.length > 1, true); + assert.equal(chunks.length > 5, true); }); it('Body of slots is chunked', async () => { From f4628b4df66fc7b669688d838cf8d1bb8a88b2eb Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Thu, 16 May 2024 09:23:47 -0400 Subject: [PATCH 5/7] Add new assertion --- packages/astro/test/streaming.test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/astro/test/streaming.test.js b/packages/astro/test/streaming.test.js index cbd4fa4f4b46..a174afe3b731 100644 --- a/packages/astro/test/streaming.test.js +++ b/packages/astro/test/streaming.test.js @@ -38,6 +38,7 @@ describe('Streaming', () => { chunks.push(chunk); } assert.equal(chunks.length > 5, true); + assert.ok(!chunks[0].includes('id="promise"'), 'the first chunk should not include the initial promise creation'); }); it('Body of slots is chunked', async () => { From dfd3d87994fe71a15910c99ec0731614e3f47219 Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Thu, 16 May 2024 12:09:39 -0400 Subject: [PATCH 6/7] Add explanation of the renderingComplete variable --- packages/astro/src/runtime/server/render/astro/render.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/astro/src/runtime/server/render/astro/render.ts b/packages/astro/src/runtime/server/render/astro/render.ts index efed9751b82a..e945551d863d 100644 --- a/packages/astro/src/runtime/server/render/astro/render.ts +++ b/packages/astro/src/runtime/server/render/astro/render.ts @@ -221,6 +221,8 @@ export async function renderToAsyncIterable( await next.promise; } + // Only create a new promise if rendering is still ongoing. Otherwise + // there will be a dangling promises that breaks tests (probably not an actual app) if(!renderingComplete) { next = promiseWithResolvers(); } From 848a3dcae30ac57e3e23621cb08b525aa4ffd878 Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Thu, 16 May 2024 12:20:22 -0400 Subject: [PATCH 7/7] Remove flaky assertion --- packages/astro/test/streaming.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/astro/test/streaming.test.js b/packages/astro/test/streaming.test.js index a174afe3b731..cbd4fa4f4b46 100644 --- a/packages/astro/test/streaming.test.js +++ b/packages/astro/test/streaming.test.js @@ -38,7 +38,6 @@ describe('Streaming', () => { chunks.push(chunk); } assert.equal(chunks.length > 5, true); - assert.ok(!chunks[0].includes('id="promise"'), 'the first chunk should not include the initial promise creation'); }); it('Body of slots is chunked', async () => {