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: throw on retry when payload is consume by downstream #3389

Merged
merged 3 commits into from
Jul 3, 2024

Conversation

climba03003
Copy link
Contributor

Close #3365 Fixes #3356

This relates to...

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

@climba03003 climba03003 changed the title fix: throw on retry when payload is consumed fix: throw on retry when payload is consume by downstream Jul 3, 2024
@climba03003 climba03003 force-pushed the throw-consumed-payload-retry branch from f947d7f to 9d41b62 Compare July 3, 2024 10:51
lib/handler/retry-handler.js Outdated Show resolved Hide resolved
lib/handler/retry-handler.js Outdated Show resolved Hide resolved
lib/handler/retry-handler.js Outdated Show resolved Hide resolved
lib/handler/retry-handler.js Outdated Show resolved Hide resolved
Comment on lines +200 to +206
this.abort(
new RequestRetryError('server does not support the range header and the payload was partially consumed', statusCode, {
headers,
data: { count: this.retryCount }
})
)
return false
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.abort(
new RequestRetryError('server does not support the range header and the payload was partially consumed', statusCode, {
headers,
data: { count: this.retryCount }
})
)
return false
throw new RequestRetryError('server does not support the range header and the payload was partially consumed', statusCode, {
headers,
data: { count: this.retryCount }
})
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure it should throw instead of similar to the others error?

Copy link
Member

Choose a reason for hiding this comment

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

I think all errors should throw

Copy link
Member

Choose a reason for hiding this comment

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

it's a nit, you can leave it if you want

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 71121e5 into nodejs:main Jul 3, 2024
30 checks passed
@climba03003 climba03003 deleted the throw-consumed-payload-retry branch July 3, 2024 17:06
github-actions bot pushed a commit that referenced this pull request Sep 12, 2024
* fix: throw on retry when payload is consumed

* fixup

* fixup

(cherry picked from commit 71121e5)
@github-actions github-actions bot mentioned this pull request Dec 3, 2024
This was referenced Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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