httploader: Implement retries #5077

yroc92 · 2022-09-24T06:52:48Z

Resolves #5070

yroc92 · 2022-09-24T06:56:17Z

Not sure if the logging is correct? I think I can replace their logger like so:

retryClient.Logger = ctx.Logger()

but I kept getting this error:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x18 pc=0x102c544ec]

goroutine 1 [running]:
github.com/caddyserver/caddy/v2.(*Logging).Logger(0x14000499860, {0x0?, 0x0?})
        /Users/corycooper/workspace/github/caddy/logging.go:202 +0x2c
github.com/caddyserver/caddy/v2.Context.Logger({{0x103f5e4f0, 0x140003b4d40}, 0x14000499800, 0x14000054f00, {0x0, 0x0, 0x0}, {0x0, 0x0, 0x0}}, ...)
        /Users/corycooper/workspace/github/caddy/context.go:478 +0xc0
github.com/caddyserver/caddy/v2/caddyconfig.getRetryClient({{0x103f5e4f0, 0x140003b4d40}, 0x14000499800, 0x14000054f00, {0x0, 0x0, 0x0}, {0x0, 0x0, 0x0}}, ...)
        /Users/corycooper/workspace/github/caddy/caddyconfig/httploader.go:131 +0xbc
github.com/caddyserver/caddy/v2/caddyconfig.HTTPLoader.makeClient({{0x140006ddf28, 0x4}, {0x14000044ae0, 0x22}, 0x0, 0x0, 0x5, 0x0}, {{0x103f5e4f0, 0x140003b4d40}, ...})
        /Users/corycooper/workspace/github/caddy/caddyconfig/httploader.go:145 +0x7c
github.com/caddyserver/caddy/v2/caddyconfig.HTTPLoader.LoadConfig({{0x140006ddf28, 0x4}, {0x14000044ae0, 0x22}, 0x0, 0x0, 0x5, 0x0}, {{0x103f5e4f0, 0x140003b4d40}, ...})
        /Users/corycooper/workspace/github/caddy/caddyconfig/httploader.go:81 +0x17c
github.com/caddyserver/caddy/v2.finishSettingUp({{0x103f5e4f0, 0x140003b4d40}, 0x14000499800, 0x14000054f00, {0x0, 0x0, 0x0}, {0x0, 0x0, 0x0}}, ...)
        /Users/corycooper/workspace/github/caddy/caddy.go:603 +0x4c0
github.com/caddyserver/caddy/v2.run(0x1400025e320?, 0x1)
        /Users/corycooper/workspace/github/caddy/caddy.go:525 +0x584
github.com/caddyserver/caddy/v2.unsyncedDecodeAndRun({0x1400025e1e0, 0x91, 0xa0}, 0x1)
        /Users/corycooper/workspace/github/caddy/caddy.go:337 +0xe4
github.com/caddyserver/caddy/v2.changeConfig({0x1037fc1a6, 0x4}, {0x1038092a9, 0x7}, {0x140002ec400, 0xe1, 0x200}, {0x0, 0x0}, 0x1)
        /Users/corycooper/workspace/github/caddy/caddy.go:228 +0x644
github.com/caddyserver/caddy/v2.Load({0x140002ec400?, 0xb?, 0x0?}, 0x0?)
        /Users/corycooper/workspace/github/caddy/caddy.go:127 +0x8c
github.com/caddyserver/caddy/v2/cmd.cmdRun({0x0?})
        /Users/corycooper/workspace/github/caddy/cmd/commandfuncs.go:212 +0x558
github.com/caddyserver/caddy/v2/cmd.caddyCmdToCoral.func1(0x140002e0500?, {0x1037fce86?, 0x2?, 0x2?})
        /Users/corycooper/workspace/github/caddy/cmd/cobra.go:114 +0x30
github.com/spf13/cobra.(*Command).execute(0x140002e0500, {0x140001c8b20, 0x2, 0x2})
        /Users/corycooper/go/pkg/mod/github.com/spf13/cobra@v1.1.3/command.go:852 +0x4c4
github.com/spf13/cobra.(*Command).ExecuteC(0x104baac40)
        /Users/corycooper/go/pkg/mod/github.com/spf13/cobra@v1.1.3/command.go:960 +0x34c
github.com/spf13/cobra.(*Command).Execute(...)
        /Users/corycooper/go/pkg/mod/github.com/spf13/cobra@v1.1.3/command.go:897
github.com/caddyserver/caddy/v2/cmd.Main()
        /Users/corycooper/workspace/github/caddy/cmd/main.go:64 +0x74
main.main()
        /Users/corycooper/workspace/github/caddy/cmd/caddy/main.go:39 +0x1c

francislavoie · 2022-09-24T11:59:34Z

Is the dependency really necessary? This seems like the sort of thing we should just do ourselves in code. We try to avoid dependencies as much as possible.

mholt · 2022-09-24T18:05:06Z

Yeah, retries can be done with a for loop.

yroc92 · 2022-09-24T21:12:44Z

So I have the client.Do() call replaced with this function, but keep getting a nil point error when trying to use ctx.Logger(). What am I missing?

// Reattempts the http call using exponential back off when maxRetries is greater than 0.
func doHttpCallWithRetries(ctx caddy.Context, client *http.Client, request *http.Request, maxRetries int) (*http.Response, error) {
	var resp *http.Response
	var err error
	var errorMessage string
	for retry := 0; retry <= maxRetries; retry++ {
		resp, err = client.Do(request)
		if err != nil {
			errorMessage = fmt.Sprintf("problem calling endpoint: %v", err)
		} else if resp.StatusCode < 200 || resp.StatusCode > 499 {
			errorMessage = fmt.Sprintf("bad response status code from endpoint: %v", resp.StatusCode)
		} else {
			return resp, nil
		}
		ctx.Logger().Error(errorMessage)

		exponentialBackOff := math.Pow(2, float64(retry))
		ctx.Logger().Info(fmt.Sprintf("reattempting in %g seconds", exponentialBackOff))
		time.Sleep(time.Duration(exponentialBackOff) * time.Second)
	}

	return resp, err
}

Error:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x18 pc=0x100ac84ec]

goroutine 1 [running]:
github.com/caddyserver/caddy/v2.(*Logging).Logger(0x140000bd800, {0x0?, 0x0?})
        /Users/corycooper/workspace/github/caddy/logging.go:202 +0x2c
github.com/caddyserver/caddy/v2.Context.Logger({{0x101dc8c70, 0x14000683780}, 0x140000bd7d0, 0x1400018fd10, {0x0, 0x0, 0x0}, {0x0, 0x0, 0x0}}, ...)
        /Users/corycooper/workspace/github/caddy/context.go:478 +0xc0
github.com/caddyserver/caddy/v2/caddyconfig.HTTPLoader.doHttpCallWithRetries({{0x14000688f58, 0x4}, {0x140001d4d50, 0x24}, 0x0, 0x0, 0x5, 0x0}, {{0x101dc8c70, 0x14000683780}, ...}, ...)
        /Users/corycooper/workspace/github/caddy/caddyconfig/httploader.go:140 +0xa4
github.com/caddyserver/caddy/v2/caddyconfig.HTTPLoader.LoadConfig({{0x14000688f58, 0x4}, {0x140001d4d50, 0x24}, 0x0, 0x0, 0x5, 0x0}, {{0x101dc8c70, 0x14000683780}, ...})
        /Users/corycooper/workspace/github/caddy/caddyconfig/httploader.go:101 +0x35c
github.com/caddyserver/caddy/v2.finishSettingUp({{0x101dc8c70, 0x14000683780}, 0x140000bd7d0, 0x1400018fd10, {0x0, 0x0, 0x0}, {0x0, 0x0, 0x0}}, ...)
        /Users/corycooper/workspace/github/caddy/caddy.go:603 +0x4c0
github.com/caddyserver/caddy/v2.run(0x140000d6320?, 0x1)
        /Users/corycooper/workspace/github/caddy/caddy.go:525 +0x584
github.com/caddyserver/caddy/v2.unsyncedDecodeAndRun({0x140000d6280, 0x93, 0xa0}, 0x1)
        /Users/corycooper/workspace/github/caddy/caddy.go:337 +0xe4
github.com/caddyserver/caddy/v2.changeConfig({0x10166b466, 0x4}, {0x101678569, 0x7}, {0x14000474400, 0xe3, 0x200}, {0x0, 0x0}, 0x1)
        /Users/corycooper/workspace/github/caddy/caddy.go:228 +0x644
github.com/caddyserver/caddy/v2.Load({0x14000474400?, 0xb?, 0x0?}, 0x0?)
        /Users/corycooper/workspace/github/caddy/caddy.go:127 +0x8c
github.com/caddyserver/caddy/v2/cmd.cmdRun({0x0?})
        /Users/corycooper/workspace/github/caddy/cmd/commandfuncs.go:212 +0x558
github.com/caddyserver/caddy/v2/cmd.caddyCmdToCoral.func1(0x140003fc000?, {0x10166c146?, 0x2?, 0x2?})
        /Users/corycooper/workspace/github/caddy/cmd/cobra.go:114 +0x30
github.com/spf13/cobra.(*Command).execute(0x140003fc000, {0x140000bef80, 0x2, 0x2})
        /Users/corycooper/go/pkg/mod/github.com/spf13/cobra@v1.1.3/command.go:852 +0x4c4
github.com/spf13/cobra.(*Command).ExecuteC(0x102a12a20)
        /Users/corycooper/go/pkg/mod/github.com/spf13/cobra@v1.1.3/command.go:960 +0x34c
github.com/spf13/cobra.(*Command).Execute(...)
        /Users/corycooper/go/pkg/mod/github.com/spf13/cobra@v1.1.3/command.go:897
github.com/caddyserver/caddy/v2/cmd.Main()
        /Users/corycooper/workspace/github/caddy/cmd/main.go:64 +0x74
main.main()
        /Users/corycooper/workspace/github/caddy/cmd/caddy/main.go:39 +0x1c
        ```

francislavoie · 2022-09-24T21:23:03Z

The logger issue is related to this change e43b6d8

But I don't know why it's failing. It seems like it's not able to get a module from the context for some reason. I'm confused. It might be because config loader modules don't get Provision'd so they don't go through the module auto-wiring stuff. Matt will need to chime in here of course.

yroc92 · 2022-09-25T06:41:24Z

I changed it to use the caddy.Log() directly which works.

mholt · 2022-09-28T20:18:38Z

I'll take a look at this shortly!

mholt

I think this looks good overall! I had some thoughts that I wanted to bounce off you.

caddyconfig/httploader.go

mholt

This is looking really good!

Most of my comments are nits but I have a few technical suggestions as well. Hopefully these make sense.

Thanks for your patience with me.

caddyconfig/httploader.go

mholt

Awesome!! Looking much better and more straightforward.

Just a few suggestions for code tidiness but otherwise this has my approval.

caddyconfig/httploader.go

Co-authored-by: Matt Holt <mholt@users.noreply.github.com>

mholt

It's perfect. Thanks Cory!

yroc92 requested a review from mholt September 24, 2022 06:52

francislavoie changed the title ~~admin:httploader: Add max_retries~~ caddyconfig: Add max_retries option to admin httploader Sep 24, 2022

francislavoie changed the title ~~caddyconfig: Add max_retries option to admin httploader~~ caddyconfig: Add max_retries option to admin http config loader Sep 24, 2022

yroc92 marked this pull request as ready for review September 25, 2022 06:36

yroc92 force-pushed the load-http-retries branch from a4739f4 to a176d4f Compare September 25, 2022 06:41

mholt requested changes Sep 29, 2022

View reviewed changes

caddyconfig/httploader.go Outdated Show resolved Hide resolved

francislavoie added the feature ⚙️ New feature or request label Oct 1, 2022

yroc92 force-pushed the load-http-retries branch from a176d4f to a2b3cad Compare October 4, 2022 03:46

yroc92 changed the title ~~caddyconfig: Add max_retries option to admin http config loader~~ caddyconfig: Add retry_delay option to admin http config loader Oct 5, 2022

yroc92 requested review from mholt and francislavoie October 5, 2022 02:18

yroc92 added 3 commits October 4, 2022 21:22

httploader: Add max_retries

203da72

caddyconfig: dependency-free http config loading retries

6a795e2

caddyconfig: support retry_delay in http loader

9f257ea

yroc92 force-pushed the load-http-retries branch from a2b3cad to 9f257ea Compare October 5, 2022 04:23

mholt requested changes Oct 5, 2022

View reviewed changes

mholt added the under review 🧐 Review is pending before merging label Oct 5, 2022

mholt added this to the v2.6.2 milestone Oct 5, 2022

httploader: Implement retries

5eb13cc

yroc92 changed the title ~~caddyconfig: Add retry_delay option to admin http config loader~~ httploader: Implement retries Oct 5, 2022

mholt approved these changes Oct 5, 2022

View reviewed changes

caddyconfig/httploader.go Outdated Show resolved Hide resolved

caddyconfig/httploader.go Outdated Show resolved Hide resolved

caddyconfig/httploader.go Outdated Show resolved Hide resolved

mholt removed the under review 🧐 Review is pending before merging label Oct 5, 2022

Apply suggestions from code review

18b8204

Co-authored-by: Matt Holt <mholt@users.noreply.github.com>

mholt approved these changes Oct 6, 2022

View reviewed changes

mholt merged commit 498f32b into master Oct 6, 2022

mholt deleted the load-http-retries branch October 6, 2022 04:34

francislavoie mentioned this pull request Oct 19, 2022

Memory leak in v2.6.2 #5158

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

httploader: Implement retries #5077

httploader: Implement retries #5077

yroc92 commented Sep 24, 2022 •

edited

yroc92 commented Sep 24, 2022 •

edited

francislavoie commented Sep 24, 2022

mholt commented Sep 24, 2022

yroc92 commented Sep 24, 2022

francislavoie commented Sep 24, 2022

yroc92 commented Sep 25, 2022

mholt commented Sep 28, 2022

mholt left a comment

mholt left a comment

mholt left a comment

mholt left a comment

httploader: Implement retries #5077

httploader: Implement retries #5077

Conversation

yroc92 commented Sep 24, 2022 • edited

yroc92 commented Sep 24, 2022 • edited

francislavoie commented Sep 24, 2022

mholt commented Sep 24, 2022

yroc92 commented Sep 24, 2022

francislavoie commented Sep 24, 2022

yroc92 commented Sep 25, 2022

mholt commented Sep 28, 2022

mholt left a comment

Choose a reason for hiding this comment

mholt left a comment

Choose a reason for hiding this comment

mholt left a comment

Choose a reason for hiding this comment

mholt left a comment

Choose a reason for hiding this comment

yroc92 commented Sep 24, 2022 •

edited

yroc92 commented Sep 24, 2022 •

edited