add ListenConfigControl option to Listener

[noboruma]

Description

Add an optional option to apply socket configurations before socket binding.

Tested locally (PASSED) with PORT_REUSE option.

[codecov]

Codecov Report

❌ Patch coverage is 53.33333% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.47%. Comparing base (accee59) to head (91eca61).

Files with missing lines	Patch %	Lines
options.go	16.66%	5 Missing ⚠️
internal/net/udp/packet_conn.go	77.77%	1 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (53.33%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #802      +/-   ##
==========================================
- Coverage   82.57%   82.47%   -0.10%     
==========================================
  Files         116      116              
  Lines        6650     6664      +14     
==========================================
+ Hits         5491     5496       +5     
- Misses        760      767       +7     
- Partials      399      401       +2     

Flag	Coverage Δ
go	82.47% <53.33%> (-0.10%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[theodorsm]

Hi @noboruma! Could you shortly explain your use-case for adding this?

I left some comments. Could you also add a test for the functionality?

Also remember to format.

[theodorsm]

Config will be deprecated. I suggest removing this, we now prefer the options-based APIs (*WithOptions).

[noboruma]

I have made this field private, *WithOptions still relies on this Config struct internally

[JoTurk]

Thank you so much for doing this fast :)

other than fixing the lint we require that all PRs include a test even if a simple one, we should just add a simple one to make sure that we don't break this in the future.

[JoTurk]

I think we should do like you suggested initially in discord with ListenConfig not just control.

[adrianosela]

+1

[noboruma]

I have replied below, please let me know if that makes sense and I will change accordingly!

[JoTurk]

@theodorsm the reason for this was discussed on discord https://discord.com/channels/1352636971591274548/1359189787768258671/1473686824701661277

[adrianosela]

For additional flexibility, why not do WithListenConfig and pass a whole net.ListenConfig?

edit: I see JoTurk suggested it above as well.

[JoTurk]

For additional flexibility, why not do WithListenConfig and pass a whole net.ListenConfig?

@adrianosela this is exactly what we agreed on in discord before making this PR, I'm curious if there a reason for going with control instead :)

[noboruma]

@JoTurk @adrianosela

this is exactly what we agreed on in discord before making this PR, I'm curious if there a reason for going with control instead :)

Indeed this is what we agreed on and this is what I started implementing but then I realized that dtls already had its own implementation of ListenConfig (here)

I thought it would be messy to embed net.ListenConfig inside that udp.ListenConfig. After looking further into it, I noticed that net.ListenConfig's other fields were only useful for TCP: KeepAlive & KeepAliveConfig - which does not apply for UDP here, so I decided to strip it down to the only useful bit: the Control bit. In a way this is making udp.ListenConfig closer to net.ListenConfig, while avoiding any future confusion by being explicit that we only care about pre-binding socket setting.

[adrianosela]

I still think the whole object is a better idea.

I think its unfortunate that udp.ListenConfig here has the same type name... but I don't think that should prevent us from doing WithListenConfig with net.ListenConfig as the argument... Maybe we can consider WithInnerListenConfig or similar...

I'm honestly not sure what's best here, but I'm not a fan of passing only Control.

Any thoughts @JoTurk , @theodorsm ? If you guys are happy with this in its current state, I don't object to it going in.