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

fix: handle calling writable.end() correctly #96

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

ezg27
Copy link
Contributor

@ezg27 ezg27 commented Nov 15, 2023

Hi there! It looks like a bug was introduced in v1.2.2 with the addition of writeFromReadableStream in this PR. I was able to reproduce this on both Node v18 and v20.

With fast responses on a fast connection the problem isn't as noticeable, but if you're serving large responses (particularly if the response returned from a Hono handler is a ReadableStream that's slow to enqueue chunks), then writable.end() in writeFromReadableStream is never called. This leads to responses in the browser showing as 'Pending' indefinitely.

I've created a reproduction repo here that demonstrates the issue - try adding a log inside the if (done) {...} statement in the flow function in node_modules/@hono/node-server/dist/index.mjs and you'll see it never gets logged. The reproduction returns an image response as a stream, with a short artificial delay inserted before each chunk is enqueued to better reproduce the problem.

The changes in this PR align more closely with this example from the Node.js documentation, where a single-use drain listener is registered with writable.once each time writable.write returns false. This should ensure that write calls are properly paused + resumed and the stream read to the end with the writable closed successfully.

Let me know if you would prefer a different approach or if there's anything else you'd like me to add to this 🙂

@yusukebe
Copy link
Member

Hi @ezg27 !

Thanks for the PR and creating the project to reproduce the bug. Looks good. I'll merge it now!

@yusukebe yusukebe merged commit efa3fe2 into honojs:main Nov 15, 2023
3 checks passed
@sjc5
Copy link

sjc5 commented Nov 15, 2023

This fixed an issue for me as well. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants