-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Isolate HTTP transports in parallel tests to prevent connection issues #3529
fix: Isolate HTTP transports in parallel tests to prevent connection issues #3529
Conversation
- Ensure each test instance uses a dedicated HTTP transport instead of sharing the default transport. - Prevents race conditions caused by CloseIdleConnections() closing connections in the shared pool. - Improves test stability by avoiding intermittent connection broken errors. References: - Related discussion: google#3527 Signed-off-by: atilsensalduz <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3529 +/- ##
==========================================
- Coverage 91.30% 91.28% -0.02%
==========================================
Files 184 184
Lines 16143 16143
==========================================
- Hits 14739 14736 -3
- Misses 1230 1231 +1
- Partials 174 176 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Thank you, @atilsensalduz!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
@gmlewis can I review it? |
Absolutely! Thank you, @Abiji-2020! We welcome all contributions. |
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.
✅ Looks Good.
I am just curious that can we reduce the timeout ?
Thank you, @Abiji-2020! The timeout here is sufficiently large such that it should NEVER trigger... and if it ever does, then something is seriously wrong. I think it is good to keep the 30-second timeout so that if it ever does take that long, then it will be pretty obvious and will alert us that it needs investigation as to why. So let's please leave it large like this for now. |
Summary
This PR addresses an issue where parallel tests share the default HTTP transport, leading to intermittent "connection broken" errors due to race conditions in connection reuse.
Root Cause
Tests create their own clients and servers via
setup(t)
, but they unintentionally share Go's default transport when no explicit transport is specified. As a result:server.Close()
triggersCloseIdleConnections()
on the default transport.Solution
References