Skip to content

fix: don't reject with unusable body when full body was received#373

Open
BlankParticle wants to merge 1 commit into
honojs:mainfrom
BlankParticle:fix/no-reject-body-unsuable
Open

fix: don't reject with unusable body when full body was received#373
BlankParticle wants to merge 1 commit into
honojs:mainfrom
BlankParticle:fix/no-reject-body-unsuable

Conversation

@BlankParticle

Copy link
Copy Markdown
Contributor

fixes #370
premature client disconnects now don't reject with TypeError: Body is unusable if full body was received
for partial body, the rejection message is also made clearer

The test case and initial patch was written by Claude Fable 5 (medium)
I reviewed the code and test cases, manually tested, rewrote some of the stuff and added the normalised abort message

@usualoma

Copy link
Copy Markdown
Member

Hi @BlankParticle,
Thanks for creating the pull request. I'm reviewing it by comparing it to what I have on my end, so please give me a moment.

@usualoma

Copy link
Copy Markdown
Member

Hi @BlankParticle,

Thank you for working on this. I confirmed that this PR fixes #370. Using the HTTP/1 parser's complete flag as a prerequisite for recovery is sound, and the change is nicely focused.

That said, a few behaviors make me hesitant to merge it as-is:

  • If the raw stream is read after the disconnect (e.g. incoming.read() via c.env.incoming) before a body method is called, the recovery path resolves with an empty body instead of rejecting. On main this case rejects with Body is unusable, so this is a behavioral regression: a raw read() on a destroyed stream doesn't set readableDidRead, so the guard doesn't catch it and the now-empty internal buffer is accepted as the full body.
  • The recovery path does not check incoming.errored before accepting the buffered body, so an application-provided stream error (e.g. a body-size guard calling req.destroy(err)) could be swallowed, causing the read to resolve instead of rejecting.
  • When setEncoding() has been used, string chunks are converted back to bytes as UTF-8 regardless of the stream's actual encoding.
  • The same disconnect scenario still fails through paths that bypass readBodyDirect, such as formData(), c.req.raw.body, and body reads that create or use the native Request cache after the disconnect.

I'd like to proceed with the fix from my existing branch instead, which also covers these cases and body-reading paths. I incorporated the parts of this PR that I found valuable in 138386d and 0007a09, and credited you as a co-author on both commits. The scenarios covered by this PR are covered on that branch as well.

I hope this approach works for you, and I'd be glad to have your review there. #375

@BlankParticle

Copy link
Copy Markdown
Contributor Author

Hi @usualoma,
thanks for checking on my PR, I made this without asking if you were working on it, I think it's fine if you can adapt parts of my PR to your existing branch. Once your PR is merged I will close this one.
I will check out and add review on your PR in a few minutes.

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.

v2: body read after client disconnect rejects with TypeError: Body is unusable even when the full body was received (regression from v1)

2 participants