[Backport 5.1] conf: Fix httpcli config#54763
Conversation
(cherry picked from commit e715340)
|
|
||
| - | ||
|
|
||
| ## 5.1.2 |
There was a problem hiding this comment.
@sourcegraph/release-guild help what did I do lmao
There was a problem hiding this comment.
cherry-pick can pull in unrelated lines because of the 3-way merge
There was a problem hiding this comment.
We aren't maintaining the changelog on the release branch anymore, so its' not a big deal
| // in this package as conf itself uses httpcli's internal client. | ||
| func EnsureHTTPClientIsConfigured() { | ||
| ensureHTTPClientIsConfiguredOnce.Do(func() { | ||
| go Watch(func() { |
There was a problem hiding this comment.
@efritz drive-by comment: I realize this is just copied from the previous implementation, but is this supposed to be called in a goroutine? Right now, this will return before the client is configured...which seems funny for a function called EnsureHTTPClientIsConfigured
There was a problem hiding this comment.
conf.Watch should be safe to call before init (according to its docstring)
There was a problem hiding this comment.
Yeah, my question isn't about safety. It's about whether the client should be configured by the time EnsureHTTPClientIsConfigured returns. Right now, it's not because of the go on line 31.
There was a problem hiding this comment.
Oh, it was originally called within a goroutine from Init above, so it might be a bad name but I think the behavior is correct. We can fix this on main.
See this comment for the source issue. Looks like #52242 introduced a condition where certain httpcli settings were not being configured in the frontend due to some cyclic package dependency-avoidance trickery in the
conf.Initfunction.This PR ensures that this setup code is hit in the frontend initialization process. No other service should need this (as they hit the
conf.Initfunction directly fromSingleServiceMainon startup), and no other feature should have been affected (as these are the only settings missing from that path).Test plan
Tested locally.
Backport e715340 from #54745