server: try avoid the "address already in use" error#8842
server: try avoid the "address already in use" error#8842mergify[bot] merged 7 commits intomasterfrom
Conversation
Replace defer l.Close() with l.Close() to shutdown the listener as fast as possible to avoid the "bind: address already in use" error that so often occurs in CI build. From: phayes/freeport#8 Reference: #6671
Codecov Report
@@ Coverage Diff @@
## master #8842 +/- ##
=======================================
Coverage 59.15% 59.15%
=======================================
Files 570 570
Lines 31770 31768 -2
=======================================
Hits 18793 18793
+ Misses 10776 10774 -2
Partials 2201 2201
|
amaury1093
left a comment
amaury1093
left a comment
There was a problem hiding this comment.
All tests seem to pass on CI, so I'm okay with this.
| panic(err) | ||
| } | ||
| if err := l.Close(); err != nil { | ||
| return "", "", fmt.Errorf("couldn't close the listener: %w", err) |
There was a problem hiding this comment.
should we wrap it?
sdkerrors.Wrap(sdkerrors.ErrIO, ....)
There was a problem hiding this comment.
Yes, sure, good one. I'm not just yet sure this PR actually fixes anything at all. I warmly welcome suggestions - and I'm CC'ing @odeke-em too.
If this does not improve anything, I'd just close it.
There was a problem hiding this comment.
The PR has good improvement - it removes panic
There was a problem hiding this comment.
[thinking aloud]
shouldn't I call Close() as the very last statement of this function before returning?
[/thinking aloud]
There was a problem hiding this comment.
Yup, @robert-zaremba got a great point that we should wrap it, for consistency with other errors.
|
Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=ec354e38040bcfe1aaad394e01ac272806277301 |
|
Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=925c74779d524a63b15eb724f5b6b961 |
|
Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=850135918558f7c43fb24fb86564c9454dcf66ea |
odeke-em
left a comment
odeke-em
left a comment
There was a problem hiding this comment.
LGTM, thank you @alessio! Good start for now and thanks for referencing that issue that I flagged a while ago. Later on would be nice for us to dig into the syscall package and figure out the calls to get an available address and bind to it then pass in a listener to the code that needs it. Currently still closing is at the mercy of the OS reusing file descriptors at a speed. Thanks again, and my apologies for the late reply.
|
Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=8877c91f9134468ca74f5b70e148324f |
|
Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=d02a39739be4850548c8c157660cb952b83e2ac7 |
6 participants
Replace defer l.Close() with l.Close() to shutdown the listener
as fast as possible to avoid the "bind: address already in use"
error that so often occurs in CI build.
From: phayes/freeport#8
Reference: #6671
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes