Skip to content

Conversation

@yjhmelody
Copy link
Contributor

@yjhmelody yjhmelody commented Nov 30, 2020

I add a test for this behavior, I think it is a bug.

@codecov
Copy link

codecov bot commented Nov 30, 2020

Codecov Report

Merging #392 (e0406c2) into master (b90c855) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #392   +/-   ##
=======================================
  Coverage   95.97%   95.97%           
=======================================
  Files          10       10           
  Lines        1019     1019           
=======================================
  Hits          978      978           
  Misses         23       23           
  Partials       18       18           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b90c855...e0406c2. Read the comment docs.

@yjhmelody yjhmelody changed the title fix: meet invalid memory when maxRetries < 0 fix: meet invalid memory when Backoff return nil Dec 3, 2020
@yjhmelody yjhmelody changed the title fix: meet invalid memory when Backoff return nil fix:when SetRetryCount -1, both resp and error are nil. Dec 3, 2020
Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

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

@yjhmelody Thanks for creating a PR. I'm sorry I'm not able to grasp the outcome.

Can you explain and help me to understand?

@yjhmelody
Copy link
Contributor Author

yjhmelody commented Jan 11, 2021

@jeevatkm

https://github.com/go-resty/resty/blob/master/retry.go#L87

for attempt := 0; attempt <= opts.maxRetries; attempt++ {
...

opts.MaxRetries is -1 so we cannot go into the loop,
The Backoff's operation has not done anything exactly. So both err and resp are nil.

@jeevatkm
Copy link
Member

@yjhmelody This PR contains new test cases added. But you mentioned it as "fix". What is the bug and fix you're submitting?

@moorereason
Copy link
Contributor

I created issue #404 to track the actual issue reported in the PR.

@yjhmelody
Copy link
Contributor Author

@moorereason Oh, let we choose the second solution ?

@jeevatkm jeevatkm added this to the v2.6.0 Milestone milestone Mar 20, 2021
@jeevatkm jeevatkm merged commit 72b25e5 into go-resty:master Mar 20, 2021
Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

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

@yjhmelody Thanks for your contributions. I'm really sorry for the delay, due to personal reasons.

@yjhmelody yjhmelody deleted the fix-maxRetries branch March 22, 2021 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants