reverseproxy: Always return new upstreams (fix #5736)

[mholt]

Should fix #5736.

I don't love this, because it involves allocating new Upstreams for every request, but there's no other elegant, safe way to do this AFAIK.

One potential solution could be to have the GetUpstreams() method return []Upstream instead of []*Upstream -- this would be a breaking change but AFAIK nobody else is implementing these modules (yet?). It would also be a correctness bugfix, since the current API returns the same upstream values but we change the Host value. (Even if synchronized, it'd logically be wrong.)

@kkroo Would you be able to give this a try and confirm whether it resolves the races for you, at least? I might still implement the fix differently and ask you to test one more time, but if we verify as we go I can be more confident of the fix. Thank you!!

Update: after thinking on this some more, I guess this is probably the best I can think of for right now. Using Upstream isn't really viable for a number of complex reasons, and while the allocations around Upstream are unfortunate, they only happen when using dynamic upstreams, which is rare.

[francislavoie]

this would be a breaking change but AFAIK nobody else is implementing these modules (yet?)

I believe there's at least a few private-source implementations of dynamic upstreams already. It's a common need to do custom tenancy-based upstream routing.

That said, did we mark it experimental? Might be a good idea to do it sooner rather than later if we need to make a change. Can we do it in a backwards compatible way, i.e. a new interface and try asserting both for now (prefer the new one) and remove the old one later?

[mholt]

It's not experimental AFAIK :(

It's OK though, I think I've decided to leave the API as-is. It's just unfortunate for modules that wish to cache their upstreams; they can still cache the dial addresses, but the actual upstreams need to be allocated each time.

[francislavoie]

Can sync.Pool be used at all?

[mholt]

Possibly! I'd need to fiddle with it. Maybe for a future PR.

[kkroo]

Thanks @mholt for the fix.

I haven’t fully tested yet, but initially it looks good. Will report in a few hours.

At first glance, it seems that DNS results cannot be cached between Upstream instances which is probably a bigger performance bottleneck than the memory allocations themselves

[mholt]

@kkroo Awesome, thanks.

At first glance, it seems that DNS results cannot be cached between Upstream instances which is probably a bigger performance bottleneck than the memory allocations themselves

I believe the results of DNS lookups for both A and SRV upstreams are still cached, we just replace the "container" we return them in each time.

[kkroo]

Thanks for the clarification.

There is still a write/write race condition on reverseproxy.go:1080 from different routines but it seems the rest of the races went away.

[mholt]

@kkroo Dang. Ok, so that's this line right?

h.HealthChecks.Passive.logger = h.logger.Named("health_checker.passive")

Or am I reading the wrong commit again? Can you give me the exact commit and full stack trace of that race?

[kkroo]

Yes, exactly. Samestack trace as in the orignal issue

EDIT: I cherry-picked the commit here on top of the PR i had open (unrelated), so line 1080 corresponds to
h.HealthChecks.Passive.logger = h.logger.Named("health_checker.passive")

I will checkout this PR directly and let you know if anything is somehow differnt

| ==================
| WARNING: DATA RACE
| Write at 0x00c000067db8 by goroutine 436:
|   github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy.Handler.provisionUpstream()
|       /root/go/src/github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy/reverseproxy.go:1080 +0x19b
|   github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy.(*Handler).proxyLoopIteration()
|       /root/go/src/github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy/reverseproxy.go:464 +0x21d7
|   github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy.(*Handler).ServeHTTP()
|       /root/go/src/github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy/reverseproxy.go:434 +0x9ac
|   github.com/caddyserver/caddy/v2/modules/caddyhttp.wrapMiddleware.func1.1()
|       /root/go/src/github.com/caddyserver/caddy/v2/modules/caddyhttp/routes.go:331 +0xe8
|   github.com/caddyserver/caddy/v2/modules/caddyhttp.HandlerFunc.ServeHTTP()
|       /root/go/src/github.com/caddyserver/caddy/v2/modules/caddyhttp/caddyhttp.go:58 +0x7e

|
|
| Previous write at 0x00c000067db8 by goroutine 427:
|   github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy.Handler.provisionUpstream()
|       /root/go/src/github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy/reverseproxy.go:1080 +0x19b
|   github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy.(*Handler).proxyLoopIteration()
|       /root/go/src/github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy/reverseproxy.go:464 +0x21d7
|   github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy.(*Handler).ServeHTTP()
|       /root/go/src/github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy/reverseproxy.go:434 +0x9ac
|   github.com/caddyserver/caddy/v2/modules/caddyhttp.wrapMiddleware.func1.1()
|       /root/go/src/github.com/caddyserver/caddy/v2/modules/caddyhttp/routes.go:331 +0xe8
|   github.com/caddyserver/caddy/v2/modules/caddyhttp.HandlerFunc.ServeHTTP()
|       /root/go/src/github.com/caddyserver/caddy/v2/modules/caddyhttp/caddyhttp.go:58 +0x7e

[kkroo]

  | ==================
  | WARNING: DATA RACE
  | Write at 0x00c00052c638 by goroutine 453:
  |   github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy.Handler.provisionUpstream()
  |       /root/go/src/github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy/reverseproxy.go:1081 +0x19b
  |   github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy.(*Handler).proxyLoopIteration()
  |       /root/go/src/github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy/reverseproxy.go:465 +0x21d7
  |   github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy.(*Handler).ServeHTTP()
  |       /root/go/src/github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy/reverseproxy.go:435 +0x9ac
  |   github.com/caddyserver/caddy/v2/modules/caddyhttp.wrapMiddleware.func1.1()
  |       /root/go/src/github.com/caddyserver/caddy/v2/modules/caddyhttp/routes.go:331 +0xe8
  |   github.com/caddyserver/caddy/v2/modules/caddyhttp.HandlerFunc.ServeHTTP()
  |       /root/go/src/github.com/caddyserver/caddy/v2/modules/caddyhttp/caddyhttp.go:58 +0x7e
  |
  |
  | Previous write at 0x00c00052c638 by goroutine 450:
  |   github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy.Handler.provisionUpstream()
  |       /root/go/src/github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy/reverseproxy.go:1081 +0x19b
  |   github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy.(*Handler).proxyLoopIteration()
  |       /root/go/src/github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy/reverseproxy.go:465 +0x21d7
  |   github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy.(*Handler).ServeHTTP()
  |       /root/go/src/github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy/reverseproxy.go:435 +0x9ac
  |   github.com/caddyserver/caddy/v2/modules/caddyhttp.wrapMiddleware.func1.1()
  |       /root/go/src/github.com/caddyserver/caddy/v2/modules/caddyhttp/routes.go:331 +0xe8
  |   github.com/caddyserver/caddy/v2/modules/caddyhttp.HandlerFunc.ServeHTTP()
  |       /root/go/src/github.com/caddyserver/caddy/v2/modules/caddyhttp/caddyhttp.go:58 +0x7e

[mholt]

@kkroo Ok, I think I fixed that one (see the newest commit). For some reason the passive health check logger was being set during provisionUpstream instead of the handler's Provision method. So I just moved that line. My tests pass -- can you verify that this fixes it for you?

FYI, I am hoping to release v2.7.4 today with a fix for HTTP/3 and this also included. If we hear back from you before then, awesome -- otherwise, I may just merge this since the fix was so simple and I'm fairly confident in it. But will love to get your experience afterward even if that's the case.

Thank you again!!

[francislavoie]

LGTM