-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Make sure to consume HTTP error response bodies #1173
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
Make sure to consume HTTP error response bodies #1173
Conversation
commit: |
| }; | ||
|
|
||
| const response = await (this._fetch ?? fetch)(this._url, init); | ||
| await response.body?.cancel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels weird cancelling the body before checking if response is okay. I'm not super clued up on this I just want to check line 620 will still work as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question.
I think both .ok and .status should still be available on the response objects after cancellation and unaffected by it.
Tried it out with this script:
(async () => {
// Fetch something that returns a body
const response = await fetch('https://httpbin.org/json');
console.log('Before cancel:');
console.log(' status:', response.status);
console.log(' ok:', response.ok);
console.log(' headers.content-type:', response.headers.get('content-type'));
// Cancel the body stream
await response.body?.cancel();
console.log('\nBody cancelled.\n');
console.log('After cancel:');
console.log(' status:', response.status);
console.log(' ok:', response.ok);
console.log(' headers.content-type:', response.headers.get('content-type'));
// Try to read the body now - this should fail
console.log('\nTrying to read body after cancel:');
try {
const text = await response.text();
console.log(' text:', text.substring(0, 50));
} catch (e) {
console.log(' ERROR:', e.message);
}
})();Which gave the following output:
Before cancel:
status: 200
ok: true
headers.content-type: application/json
Body cancelled.
After cancel:
status: 200
ok: true
headers.content-type: application/json
Trying to read body after cancel:
ERROR: Body is unusable: Body has already been read
So this looks good to me.
We could put 2 cancels, one inside the 405 check and then one at the end of the function, but this seems fine.
|
|
||
| // If the response is 202 Accepted, there's no body to process | ||
| if (response.status === 202) { | ||
| await response.body?.cancel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome. This line I have been meaning to add for ages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a script and not part of the sdk. can remove changes to this :)
mattzcarey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question but strong approval when answered
|
The context for this is that Cloudflare Durable Objects have a 6 simultaneous connection limit which is often reached by MCP clients. This should help us use the full amount with active requests. |
Calling body.cancel() then response.text() would fail because the stream is already closed. Use text() first to consume the body.
This script runs in Node.js at build time, not in Cloudflare Workers, so it doesn't need the body consumption fix.
|
@mattzcarey pushed some small clean-ups to this branch, feel free to accept and merge if you're happy with it |
|
I think I need a review from the auth team @pcarleton :) |
…#1173) Co-authored-by: Eduardo Gomes <egomes@cloudflare.com> Co-authored-by: Felix Weinberger <fweinberger@anthropic.com> Co-authored-by: Felix Weinberger <3823880+felixweinberger@users.noreply.github.com>
Addresses #1172
Motivation and Context
Fix the errors in Cloudflare workers when multiple requests go inflight without reading response bodies, which are breaking my SDK client flows :(
How Has This Been Tested?
Unit tests and manually tested with Cloudflare workers
Types of changes
Checklist