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: retry handler provide invalid body with fetch #3365

Closed
wants to merge 2 commits into from

Conversation

climba03003
Copy link
Contributor

@climba03003 climba03003 commented Jun 24, 2024

This relates to...

Fixes #3356

Rationale

RetryHandler should buffer the body instead of streaming to the next handler.
The reason behind is that the body maybe used by the next handler immediately and leads to stacking multiple invalid response.

Changes

Buffer the body until the response is success or error.
It should result on all status code that is not 206 to retain the current behavior.

Features

Bug Fixes

Breaking Changes and Deprecations

I though it may increase in memory usage for non-fetch people, because the payload can be streamed before but not now.
But in order to retain the correct behavior for retry, buffering should happen.

Status

@climba03003 climba03003 changed the title fix: retry handler provide invalid with fetch fix: retry handler provide invalid body with fetch Jun 24, 2024
@climba03003
Copy link
Contributor Author

Test failure seems unrelated to the PR, it failed to download Node.js.

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not be buffering in memory.

Copy link
Member

@metcoder95 metcoder95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's better throw, is safer and imposes less overhead, wdyt?

@mcollina
Copy link
Member

Yes

@climba03003
Copy link
Contributor Author

See #3389

@climba03003 climba03003 closed this Jul 3, 2024
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.

The RetryHandler receives a duplicate body when the server does not support Range requests.
4 participants