fix(jsx): Fix "Invalid state: Controller is already closed"#4770
Merged
yusukebe merged 3 commits intohonojs:mainfrom Mar 4, 2026
Merged
fix(jsx): Fix "Invalid state: Controller is already closed"#4770yusukebe merged 3 commits intohonojs:mainfrom
yusukebe merged 3 commits intohonojs:mainfrom
Conversation
This was referenced Feb 25, 2026
Member
|
@gaearon Hey Dan, Thank you for the PR. The issue #4769 you faced, and honojs/node-server#233 are different problems, and we should think about them separately (right? @usualoma ). But I think this fix is resolvable and acceptable. @usualoma What do you think of this? |
Contributor
Author
|
Ah maybe that's a different message and I got them confused |
Member
|
Hi @gaearon Would you please consider the following changes? Continue processing until the endCurrently, there is an inconsistency in whether “processing is interrupted” or “continues until completion” at the following two points.
To unify this with the latter approach, I would like to implement it as follows. diff --git i/src/jsx/streaming.ts w/src/jsx/streaming.ts
index 0c0c5b2d..e21d40c7 100644
--- i/src/jsx/streaming.ts
+++ w/src/jsx/streaming.ts
@@ -158,10 +158,9 @@ export const renderToReadableStream = (
true,
context
)
- if (cancelled) {
- return
+ if (!cancelled) {
+ controller.enqueue(textEncoder.encode(resolved))
}
- controller.enqueue(textEncoder.encode(resolved))
let resolvedCount = 0
const callbacks: Promise<void>[] = []Simplify TestingTo minimize the use of try blocks within test code and delays via diff --git i/src/jsx/streaming.test.tsx w/src/jsx/streaming.test.tsx
index 3c7f53b7..75c70cf8 100644
--- i/src/jsx/streaming.test.tsx
+++ w/src/jsx/streaming.test.tsx
@@ -887,55 +887,38 @@ d.replaceWith(c.content)
const onRejection = (e: unknown) => unhandled.push(e)
process.on('unhandledRejection', onRejection)
- try {
- const SubContent = () => {
- const content = new Promise<HtmlEscapedString>((resolve) =>
- setTimeout(() => resolve(<h2>World</h2>), 50)
- )
- return content
- }
+ const SubContent = async () => <h2>World</h2>
+ const Content = async () => (
+ <>
+ <h1>Hello</h1>
+ <Suspense fallback={<p>Loading sub...</p>}>
+ <SubContent />
+ </Suspense>
+ </>
+ )
- const Content = () => {
- const content = new Promise<HtmlEscapedString>((resolve) =>
- setTimeout(
- () =>
- resolve(
- <>
- <h1>Hello</h1>
- <Suspense fallback={<p>Loading sub...</p>}>
- <SubContent />
- </Suspense>
- </>
- ),
- 20
- )
- )
- return content
- }
+ const onError = vi.fn()
+ const stream = renderToReadableStream(
+ <Suspense fallback={<p>Loading...</p>}>
+ <Content />
+ </Suspense>,
+ onError
+ )
- const onError = vi.fn()
- const stream = renderToReadableStream(
- <Suspense fallback={<p>Loading...</p>}>
- <Content />
- </Suspense>,
- onError
- )
+ const reader = stream.getReader()
+ const firstChunk = await reader.read()
+ expect(firstChunk.done).toBe(false)
- const reader = stream.getReader()
- const firstChunk = await reader.read()
- expect(firstChunk.done).toBe(false)
+ // Simulate client disconnect
+ await reader.cancel()
- // Simulate client disconnect
- await reader.cancel()
+ // Wait for nested Suspense callbacks to fire against the closed controller
+ await new Promise((resolve) => setTimeout(resolve))
- // Wait for nested Suspense callbacks to fire against the closed controller
- await new Promise((resolve) => setTimeout(resolve, 200))
+ expect(unhandled).toHaveLength(0)
+ expect(onError).not.toHaveBeenCalled()
- expect(unhandled).toHaveLength(0)
- } finally {
- process.off('unhandledRejection', onRejection)
- suspenseCounter++
- }
+ process.off('unhandledRejection', onRejection)
})
it('should not call onError when reader is cancelled during a slow callback resolution', async () => {
@@ -943,51 +926,42 @@ d.replaceWith(c.content)
const onRejection = (e: unknown) => unhandled.push(e)
process.on('unhandledRejection', onRejection)
- try {
- let signalCallbackStarted!: () => void
- const callbackStarted = new Promise<void>((r) => {
- signalCallbackStarted = r
- })
+ let signalCallbackStarted!: () => void
+ const callbackStarted = new Promise<void>((r) => {
+ signalCallbackStarted = r
+ })
- const Content = () => {
- return new Promise<HtmlEscapedString>((resolve) => {
- setTimeout(() => {
- const html = raw('<p>content</p>', [
- ((opts: any) => {
- if (opts.phase === HtmlEscapedCallbackPhase.BeforeStream) {
- signalCallbackStarted()
- return new Promise<string>((r) => setTimeout(() => r(''), 50))
- }
- return undefined
- }) as any,
- ])
- resolve(html as unknown as HtmlEscapedString)
- }, 10)
- })
- }
+ const Content = async () =>
+ raw('<p>content</p>', [
+ ((opts: any) => {
+ if (opts.phase === HtmlEscapedCallbackPhase.BeforeStream) {
+ signalCallbackStarted()
+ return new Promise<string>((r) => setTimeout(() => r('')))
+ }
+ return undefined
+ }) as any,
+ ])
- const onError = vi.fn()
- const stream = renderToReadableStream(
- <Suspense fallback={<p>Loading...</p>}>
- <Content />
- </Suspense>,
- onError
- )
+ const onError = vi.fn()
+ const stream = renderToReadableStream(
+ <Suspense fallback={<p>Loading...</p>}>
+ <Content />
+ </Suspense>,
+ onError
+ )
- const reader = stream.getReader()
- await reader.read()
+ const reader = stream.getReader()
+ await reader.read()
- await callbackStarted
- await reader.cancel()
+ await callbackStarted
+ await reader.cancel()
- await new Promise((resolve) => setTimeout(resolve, 200))
+ await new Promise((resolve) => setTimeout(resolve))
- expect(onError).not.toHaveBeenCalled()
- expect(unhandled).toHaveLength(0)
- } finally {
- process.off('unhandledRejection', onRejection)
- suspenseCounter++
- }
+ expect(unhandled).toHaveLength(0)
+ expect(onError).not.toHaveBeenCalled()
+
+ process.off('unhandledRejection', onRejection)
})
it('should not throw when cancelled before initial content resolves', async () => {
@@ -995,24 +969,20 @@ d.replaceWith(c.content)
const onRejection = (e: unknown) => unhandled.push(e)
process.on('unhandledRejection', onRejection)
- try {
- const onError = vi.fn()
- const stream = renderToReadableStream(
- new Promise<HtmlEscapedString>((resolve) =>
- setTimeout(() => resolve(raw('<p>slow content</p>') as HtmlEscapedString), 50)
- ),
- onError
- )
+ const onError = vi.fn()
+ const stream = renderToReadableStream(
+ Promise.resolve(raw('<p>slow content</p>') as HtmlEscapedString),
+ onError
+ )
- const reader = stream.getReader()
- await reader.cancel()
+ const reader = stream.getReader()
+ await reader.cancel()
- await new Promise((resolve) => setTimeout(resolve, 200))
+ await new Promise((resolve) => setTimeout(resolve))
- expect(onError).not.toHaveBeenCalled()
- expect(unhandled).toHaveLength(0)
- } finally {
- process.off('unhandledRejection', onRejection)
- }
+ expect(unhandled).toHaveLength(0)
+ expect(onError).not.toHaveBeenCalled()
+
+ process.off('unhandledRejection', onRejection)
})
}) |
yusukebe
changed the title
Co-authored-by: Taku Amano <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4770 +/- ##
==========================================
- Coverage 91.48% 91.46% -0.02%
==========================================
Files 177 177
Lines 11551 11579 +28
Branches 3353 3368 +15
==========================================
+ Hits 10567 10591 +24
- Misses 983 987 +4
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
Member
|
@yusukebe Thank you |
Member
Contributor
Author
|
thanks for the changes! i'm not very familiar with streams myself so i don't have opinions about what's the best way to do this |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4769
The fix is authored by Claude. I steered it to make failing tests first.
All three tests are failing before the fix and passing after the fix.