Skip to content

Conversation

tty2
Copy link

@tty2 tty2 commented Sep 8, 2023

baseRetryPolicy is accessable from public DefaultRetryPolicy and ErrorPropagatedRetryPolicy and it is called without any checks.
There is possible case with call this function with passing nil *http.Response inside and catch panic.
With this changes they get error with explanation.

@tty2 tty2 requested review from a team, marianoasselborn and randyhdev September 8, 2023 19:31
@hashicorp-cla
Copy link

hashicorp-cla commented Sep 8, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@manicminer manicminer left a comment

Choose a reason for hiding this comment

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

@tty2 Thanks for submitting this PR. I agree we should be nil-checking the response, however I'd suggest that the request should be retried in that circumstance, since a nil response is often attributed to the connection being closed or the server misbehaving - both temporary conditions that IMO we should smooth over.

@manicminer manicminer added the bug label May 9, 2024
tty2 and others added 3 commits May 13, 2024 18:38
@tty2 tty2 requested a review from a team as a code owner May 13, 2024 17:48
@tty2 tty2 requested a review from manicminer May 13, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants