- Notifications
You must be signed in to change notification settings - Fork1k
fix: use new http transport for webhook handler#19462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
… pool, avoiding conflicts over shared connectionsSigned-off-by: Danny Kopping <dannykopping@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we have some kind of internal "helper" library, perhaps something similar to github.com/hashicorp/go-cleanhttp?
I like that idea. It'll be quite a sizeable/wide-ranging refactor I'd imagine, and out of scope for this PR, but probably something we should look into in the future. Thanks for the quick review! |
338e8b5
intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Closescoder/internal#919
I believe that
Error "request failed: Post \"http://127.0.0.1:46685\": net/http: HTTP/1.x transport connection broken: http: CloseIdleConnections called" does not contain "non-2xx response (500)"
indicates that the client is closing what it considers to be an idle connection while another goroutine is using it. This is likely caused by another test (running concurrently) shutting down its http server while this test was still running, and both shared a reference to the globalhttp.DefaultTransport
.http.Transport
maintains a connection pool.Now we'll be cloning
http.DefaultTransport
whenever a new webhook handler gets created, ensuring that it maintains its own conn pool.I've added some defensiveness for the type inference, but it's highly unlikely that will change; in which case let's just revert to previous behaviour and log. I considered copying the panic pattern from
tailnet.go
, but since this webhook feature is optional, I think it's better to choose a slightly racy implementation than crash the server.