Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #900 +/- ##
==========================================
- Coverage 88.37% 88.18% -0.19%
==========================================
Files 44 45 +1
Lines 5591 5605 +14
==========================================
+ Hits 4941 4943 +2
- Misses 451 461 +10
- Partials 199 201 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
FrantaBOT
force-pushed
the
fix-newudpmuxdefault-panic
branch
from
e2a5c24 to
00c255f
Compare
FrantaBOT
force-pushed
the
fix-newudpmuxdefault-panic
branch
from
00c255f to
0072963
Compare
| var localAddrsForUnspecified []net.Addr | ||
| if udpAddr, ok := params.UDPConn.LocalAddr().(*net.UDPAddr); !ok { //nolint:nestif | ||
| //nolint:nestif | ||
| if params.UDPConn == nil { |
There was a problem hiding this comment.
This seems an odd way of going around UDPConn being nil. You end up creating a broken mux. Could you share more context on what's causing UDPConn to be nil in the first place? This seems like it should be handled on the caller
There was a problem hiding this comment.
This is just because we can't introduce err to the constructor without breaking the API, user/dev errors shouldn't cause a panic, I think @FrantaBOT 's workaround is okay, idk.
There was a problem hiding this comment.
Could you share more context on what's causing UDPConn to be nil in the first place?
I want to prevent panic crash from improper use by any developer.
You end up creating a broken mux
Mux returning error is better outcome then panicking.
FrantaBOT
force-pushed
the
fix-newudpmuxdefault-panic
branch
from
0072963 to
672963f
Compare
JoTurk
left a comment
JoTurk
left a comment
There was a problem hiding this comment.
What if we make a new mux constructor with options patterns, and with errors, and deprecate this one?
|
Yeah I like constructor with options patterns much better, this to me seems like we are hiding a problem and moving along with a semi-broken system? I don't know enough about the expected behavior from the system though |
3 participants
Description
Fixes a edge case where attempting to call the NewUDPMuxDefault function with UDPConn being nil it would cause a panic.
Unfortunately, this function does not return an error and never returns nil, so there is no clearway to prevent this.