Improve Retry Logic to Only Retry on Server-Side HTTP Errors #1390

VishalGawade1 · 2024-11-09T03:36:15Z

Changes Implemented

Fixes #861

Selective Retrying in osv.go:

Before: The retry logic did not differentiate between server-side and client-side HTTP errors, potentially leading to unnecessary retries on HTTP 4xx responses.
After: Updated the retry mechanism to only retry when the response status code is in the 500 range (HTTP 5xx). This prevents the system from retrying requests that are likely to fail due to client-side issues, thereby optimizing performance and reducing redundant network calls.

osv_test.go:
Verified that the updated retry logic correctly differentiates between HTTP 5xx and HTTP 4xx responses.
Ensured that retries are only attempted for HTTP 5xx errors by running and passing the TestRetryOn5xx test case.

google-cla · 2024-11-09T03:36:19Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

G-Rath

Thanks, I've not reviewed it in full as I'm on mobile, but first we should refactor the tests to not use a third party assertion library

G-Rath · 2024-11-09T05:25:22Z

pkg/osv/osv_test.go

+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"


Let's use the std libs for asserting rather than pull in a new package

I've switched to standard library assertions for simplicity. Let me know if there’s anything more to address. Thanks!

G-Rath

looks like a good start - can you also make sure you've run scripts/run_lints.sh and address anything it brings up?

G-Rath · 2024-11-10T18:03:16Z

go.mod

issue: this needs to be reverted

I ran the scripts/run_lints.sh script, and the only issues flagged were spacing warnings outside of the code I wrote. Let me know if you'd like me to address those too.

G-Rath · 2024-11-10T18:11:17Z

pkg/osv/osv.go

 func makeRetryRequest(action func() (*http.Response, error)) (*http.Response, error) {
 	var resp *http.Response
 	var err error

-	for i := range maxRetryAttempts {


issue: I think you've changed this loop more than you need to

for i := range maxRetryAttempts should be equivalent to what you've got currently

you've removed the random jitter, which is intentionally present to smooth the back-off

Yeah, you were right. Sorry for removing Jitter in the function. Added back!

G-Rath · 2024-11-10T18:13:24Z

pkg/osv/osv.go

@@ -158,7 +158,7 @@ func chunkBy[T any](items []T, chunkSize int) [][]T {

 // checkResponseError checks if the response has an error.
 func checkResponseError(resp *http.Response) error {
-	if resp.StatusCode == http.StatusOK {
+	if resp.StatusCode >= 200 && resp.StatusCode < 300 {


suggestion: I think we shouldn't change this line, since it's not needed for addressing #861

even though 2xx are success codes, we're only expecting the API currently to return a 200 and since this change isn't strictly needed for the rest of the change to work, I would lean towards not changing it for now

I've been cargo-culting the HTTP retry logic from the examples in https://github.com/sethvargo/go-retry/blob/main/retry_test.go, for what it's worth, which treats 5xx errors as retryable, only. That said, I also recently saw https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/429 in the wild, which could be retried with an exponential backoff approach.

G-Rath · 2024-11-10T18:17:54Z

pkg/osv/osv_test.go

+	// Override the QueryEndpoint for testing
+	originalQueryEndpoint := QueryEndpoint
+	QueryEndpoint = server.URL
+	defer func() { QueryEndpoint = originalQueryEndpoint }()


issue: there's no reason to do this, because QueryEndpoint isn't used directly in the function you're testing

(that'll then mean you can make this test a parallel one)

G-Rath · 2024-11-10T18:19:37Z

pkg/osv/osv_test.go

+	"time"
+)
+
+func TestRetryOn5xx(t *testing.T) {


issue: we should be testing for a couple of different status codes and orders

I'd recommend refactoring this into a table-based test, and we should have cases for at least:

200

4xx

5xx

5xx, then a 2xx

G-Rath · 2024-11-10T18:20:16Z

pkg/osv/osv_test.go

+
+	log.Printf("TestRetryOn5xx: resp = %v, err = %v", resp, err)
+
+	// Assertion: resp should be nil


nit: these comments feel pretty redundant

VishalGawade1 · 2024-11-10T22:12:12Z

Hi @G-Rath,
While running run_lints.sh, I found couple of issues, so I made some updates to the makeRetryRequest function and adjusted the osv_test.go file based on your feedback. Now, the script runs without any issues on my end, aside from a few spacing warnings in other functions. Just let me know if you’d like me to fix those up too.

G-Rath · 2024-11-10T23:55:17Z

aside from a few spacing warnings in other functions

would you mind posting a couple of those here? as there shouldn't be any output, and my local doesn't give any

VishalGawade1 · 2024-11-11T02:43:29Z

I'm not sure if this is what you are doing but If you're trying to run the script in PowerShell, it likely won’t work and will just return a blank output. Try using Git Bash instead.
here is the command from script:

go install github.com/golangci/golangci-lint/cmd/[email protected]
golangci-lint run ./... --max-same-issues 0

and here are the warnings:
pkg\osv\osv.go:160:6: func checkResponseError is unused (unused)
pkg\osv\osv.go:322:28: G404: Use of weak random number generator (math/rand or math/rand/v2 instead of crypto/rand)
pkg\osv\osv_test.go:53:3: The copy of the 'for' variable "tc" can be deleted (Go 1.22+) (copyloopvar)
pkg\spdx\verify.go:1: File is not gofmt-ed with -s (gofmt)

I'm fairly certain that running go fmt should resolve these warnings. I'll go ahead and fix the warnings related to osv_test.go. Let me know if you'd like me to address the other warnings as well.

G-Rath · 2024-11-11T02:48:24Z

@VishalGawade1 only the last one is about spacing, the others are legitimate issues to be addressed in your pull request, along with other changes from my review that you're yet to make such as restoring the for i := range loop.

The pkg\spdx\verify.go warning is interesting - I'm happy if you want to just apply that and push it up so we can take a look at what actually gets changed, even though it won't belong in this PR

If you're trying to run the script in PowerShell

I'm not since the script is a shell script not a powershell script 😅

edit: right I misread - yeah I develop in WSL so it's all going through linux; by "blank output" I mean it will give you this:

osv-scanner on  main [$?] via 🐹 v1.22.7 via  v20.11.0 via 🐍 v3.10.12 took 2s
❯ scripts/run_lints.sh
+ go run github.com/golangci/golangci-lint/cmd/[email protected] run ./... --max-same-issues 0

osv-scanner on  main [$?] via 🐹 v1.22.7 via  v20.11.0 via 🐍 v3.10.12 took 10s
❯

VishalGawade1 · 2024-11-11T02:54:19Z

@VishalGawade1 only the last one is about spacing, the others are legitimate issues to be addressed in your pull request, along with other changes from my review that you're yet to make such as restoring the for i := range loop.

The pkg\spdx\verify.go warning is interesting - I'm happy if you want to just apply that and push it up so we can take a look at what actually gets changed, even though it won't belong in this PR

If you're trying to run the script in PowerShell

I'm not since the script is a shell script not a powershell script 😅

@VishalGawade1 only the last one is about spacing, the others are legitimate issues to be addressed in your pull request, along with other changes from my review that you're yet to make such as restoring the for i := range loop.

The pkg\spdx\verify.go warning is interesting - I'm happy if you want to just apply that and push it up so we can take a look at what actually gets changed, even though it won't belong in this PR

If you're trying to run the script in PowerShell

I'm not since the script is a shell script not a powershell script 😅

edit: right I misread - yeah I develop in WSL so it's all going through linux; by "blank output" I mean it will give you this:
osv-scanner on  main [$?] via 🐹 v1.22.7 via  v20.11.0 via 🐍 v3.10.12 took 2s
❯ scripts/run_lints.sh
+ go run github.com/golangci/golangci-lint/cmd/[email protected] run ./... --max-same-issues 0

osv-scanner on  main [$?] via 🐹 v1.22.7 via  v20.11.0 via 🐍 v3.10.12 took 10s
❯

yeah, I assumed just to be safe 😄
Sure, I will take look at other warnings and will update you.

VishalGawade1 · 2024-11-11T03:04:03Z

@VishalGawade1 only the last one is about spacing, the others are legitimate issues to be addressed in your pull request, along with other changes from my review that you're yet to make such as restoring the for i := range loop.
The pkg\spdx\verify.go warning is interesting - I'm happy if you want to just apply that and push it up so we can take a look at what actually gets changed, even though it won't belong in this PR

If you're trying to run the script in PowerShell

I'm not since the script is a shell script not a powershell script 😅
@VishalGawade1 only the last one is about spacing, the others are legitimate issues to be addressed in your pull request, along with other changes from my review that you're yet to make such as restoring the for i := range loop.
The pkg\spdx\verify.go warning is interesting - I'm happy if you want to just apply that and push it up so we can take a look at what actually gets changed, even though it won't belong in this PR

If you're trying to run the script in PowerShell

I'm not since the script is a shell script not a powershell script 😅
edit: right I misread - yeah I develop in WSL so it's all going through linux; by "blank output" I mean it will give you this:
osv-scanner on  main [$?] via 🐹 v1.22.7 via  v20.11.0 via 🐍 v3.10.12 took 2s
❯ scripts/run_lints.sh
+ go run github.com/golangci/golangci-lint/cmd/[email protected] run ./... --max-same-issues 0

osv-scanner on  main [$?] via 🐹 v1.22.7 via  v20.11.0 via 🐍 v3.10.12 took 10s
❯
yeah, I assumed just to be safe 😄 Sure, I will take look at other warnings and will update you.

@G-Rath
I need your suggestion, what should I do with the checkResponse function? Should I just remove it? Is that a good Idea?

G-Rath · 2024-11-11T03:12:05Z

I need your suggestion, what should I do with the checkResponse function? Should I just remove it? Is that a good Idea?

Generally speaking I think your current implementation is a bit ... lets say nil-y (I don't want to say "messy" as I wouldn't go that far, but.. there are a lot of != nil checks in there), and I would recommend you review that to see how you could clean it up.

I believe that's why the checkResponse function existed in the first place, and it's why you've added getStatusCode - ultimately, I think the end solution will probably benefit from one utility function that is likely a combo of both of those functions, and that's why I recommend you review the whole implementation of the loop.

(that is to say, technically "yes it should go because it's not being used anymore", but I might as well frontfoot the introduction of a new similar function while we're at it 🙂)

VishalGawade1 · 2024-11-11T06:33:24Z

I need your suggestion, what should I do with the checkResponse function? Should I just remove it? Is that a good Idea?

Generally speaking I think your current implementation is a bit ... lets say nil-y (I don't want to say "messy" as I wouldn't go that far, but.. there are a lot of != nil checks in there), and I would recommend you review that to see how you could clean it up.

I believe that's why the checkResponse function existed in the first place, and it's why you've added getStatusCode - ultimately, I think the end solution will probably benefit from one utility function that is likely a combo of both of those functions, and that's why I recommend you review the whole implementation of the loop.

(that is to say, technically "yes it should go because it's not being removed anymore", but I might as well frontfoot the introduction of a new similar function while we're at it 🙂)

I totally understand, sorry about that, and I really appreciate your feedback. I'll work on making the code cleaner and more readable. I'll remove getStatusCode and checkResponse and incorporate error handling directly into makeRetryRequest. I’ll also make the test cases a bit more comprehensive and address the warnings. I'll update you soon!

…, and cleared all lint warnings

VishalGawade1 · 2024-11-11T20:26:09Z

Hi @G-Rath ,
I’ve removed both checkResponse and getStatusCode functions and consolidated error handling directly within makeRetryRequest. Additionally, there are no warnings when running run_lints. Let me know if this approach works for you. 😄

another-rex · 2024-11-11T20:26:53Z

go.mod

@@ -26,6 +26,7 @@ require (
 	github.com/package-url/packageurl-go v0.1.3
 	github.com/pandatix/go-cvss v0.6.2
 	github.com/spdx/tools-golang v0.5.5
+	github.com/stretchr/testify v1.9.0


Is this still necessary here? It looks like this is no longer being used.

another-rex · 2024-11-11T20:28:33Z

pkg/osv/osv.go

@@ -16,7 +16,7 @@ import (
 	"golang.org/x/sync/errgroup"
 )

-const (
+var (


Is this change intentional?

I changed these from const to var so previously I could adjust the endpoint, I will revert back to const

another-rex · 2024-11-11T20:29:24Z

pkg/osv/osv.go

 func makeRetryRequest(action func() (*http.Response, error)) (*http.Response, error) {
-	var resp *http.Response
-	var err error
+	const maxRetryAttempts = 4


These two variables already exist at the top const section

yeah, I kept changing the variable so i wanted to use it locally. I will revert these changes as well

another-rex · 2024-11-11T20:31:47Z

pkg/osv/osv.go

 				break
 			}
+			backoff := time.Duration(i*i) * time.Second


It's not clear why jitter is changed here, the new implementation requires duplicating the jitter code? Can you revert this to the previous implementation

…ormat, and restored jitter implementation to avoid code duplication in makeRetryRequest function

VishalGawade1 · 2024-11-11T22:12:21Z

Please review the changes. Due to the merge conflict, some modifications were added back during the restore in previous commit

VishalGawade1 added 2 commits November 8, 2024 21:17

Adding error handling for status code 5xx

d8d00bc

resolving merge conflict in go.mod

55f448a

updating osv.go comment for handling 200 inside makeRetryRequest

c91540b

G-Rath suggested changes Nov 9, 2024

View reviewed changes

refactor: use standard library for test assertions

03db161

G-Rath suggested changes Nov 10, 2024

View reviewed changes

VishalGawade1 added 2 commits November 10, 2024 15:52

fix: correct makeRetryRequest retry logic and update tests

a30e28e

Removing the QueryEndpoint Override from test

e297913

Added error handling in makeRetryRequest, removed redundant functions…

8c369d6

…, and cleared all lint warnings

another-rex reviewed Nov 11, 2024

View reviewed changes

removed unused dependency in go.mod, reverted constants to original f…

e81d59d

…ormat, and restored jitter implementation to avoid code duplication in makeRetryRequest function

Merge branch 'main' into main

f8676b7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Retry Logic to Only Retry on Server-Side HTTP Errors #1390

Improve Retry Logic to Only Retry on Server-Side HTTP Errors #1390

VishalGawade1 commented Nov 9, 2024

google-cla bot commented Nov 9, 2024

G-Rath left a comment

G-Rath Nov 9, 2024

VishalGawade1 Nov 10, 2024 •

edited

Loading

G-Rath left a comment

G-Rath Nov 10, 2024

VishalGawade1 Nov 10, 2024

G-Rath Nov 10, 2024

VishalGawade1 Nov 10, 2024

G-Rath Nov 10, 2024

andrewpollock Nov 11, 2024

G-Rath Nov 10, 2024

G-Rath Nov 10, 2024

G-Rath Nov 10, 2024

VishalGawade1 commented Nov 10, 2024

G-Rath commented Nov 10, 2024

VishalGawade1 commented Nov 11, 2024

G-Rath commented Nov 11, 2024 •

edited

Loading

VishalGawade1 commented Nov 11, 2024

VishalGawade1 commented Nov 11, 2024

G-Rath commented Nov 11, 2024 •

edited

Loading

VishalGawade1 commented Nov 11, 2024

VishalGawade1 commented Nov 11, 2024

another-rex Nov 11, 2024

another-rex Nov 11, 2024

VishalGawade1 Nov 11, 2024

another-rex Nov 11, 2024

VishalGawade1 Nov 11, 2024

another-rex Nov 11, 2024

VishalGawade1 commented Nov 11, 2024 •

edited

Loading


		log.Printf("TestRetryOn5xx: resp = %v, err = %v", resp, err)

		// Assertion: resp should be nil

Improve Retry Logic to Only Retry on Server-Side HTTP Errors #1390

Are you sure you want to change the base?

Improve Retry Logic to Only Retry on Server-Side HTTP Errors #1390

Conversation

VishalGawade1 commented Nov 9, 2024

google-cla bot commented Nov 9, 2024

G-Rath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VishalGawade1 Nov 10, 2024 • edited Loading

Choose a reason for hiding this comment

G-Rath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VishalGawade1 commented Nov 10, 2024

G-Rath commented Nov 10, 2024

VishalGawade1 commented Nov 11, 2024

G-Rath commented Nov 11, 2024 • edited Loading

VishalGawade1 commented Nov 11, 2024

VishalGawade1 commented Nov 11, 2024

G-Rath commented Nov 11, 2024 • edited Loading

VishalGawade1 commented Nov 11, 2024

VishalGawade1 commented Nov 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VishalGawade1 commented Nov 11, 2024 • edited Loading

VishalGawade1 Nov 10, 2024 •

edited

Loading

G-Rath commented Nov 11, 2024 •

edited

Loading

G-Rath commented Nov 11, 2024 •

edited

Loading

VishalGawade1 commented Nov 11, 2024 •

edited

Loading