Skip to content
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

proposal: net: add Listen and Dial for TCP and UDP that takes an AddrPort #49522

Open
zx2c4 opened this issue Nov 11, 2021 · 8 comments
Open

proposal: net: add Listen and Dial for TCP and UDP that takes an AddrPort #49522

zx2c4 opened this issue Nov 11, 2021 · 8 comments
Labels
Projects
Milestone

Comments

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Nov 11, 2021

In Go 1.18, we added Read and Write functions for UDP that work on AddrPort instead of UDPAddr. It didn't make sense to do the same for TCP, because TCP's Read and Write functions don't take or return IP addresses, since TCP is always bound.

However, left out of both TCP and UDP for 1.18 was AddrPort functions for Listen and Dial. This proposal is to add those for Go 1.19. Specifically:

  • Existing: func DialTCP(network string, laddr, raddr *TCPAddr) (*TCPConn, error)
  • New: func DialTCPAddrPort(network string, laddr, raddr netip.AddrPort) (*TCPConn, error)
    _
  • Existing: func ListenTCP(network string, laddr *TCPAddr) (*TCPListener, error)
  • New: func ListenTCPAddrPort(network string, laddr netip.AddrPort) (*TCPListener, error)
    _
  • Existing: func DialUDP(network string, laddr, raddr *UDPAddr) (*UDPConn, error)
  • New: func DialUDPAddrPort(network string, laddr, raddr netip.AddrPort) (*UDPConn, error)
    _
  • Existing: func ListenUDP(network string, laddr *UDPAddr) (*UDPConn, error)
  • New: func ListenUDPAddrPort(network string, laddr netip.AddrPort) (*UDPConn, error)
    _
  • Existing: func ListenMulticastUDP(network string, ifi *Interface, gaddr *UDPAddr) (*UDPConn, error)
  • New: func ListenMulticastUDPAddrPort(network string, ifi *Interface, gaddr netip.AddrPort) (*UDPConn, error)

(The naming scheme, of tacking on AddrPort to the previous function name follows what was already done for Go 1.18 with, e.g., WriteMsgUDP -> WriteMsgUDPAddrPort. )

An implementation of this is available in CL363374.


CC @bradfitz @crawshaw @josharian @danderson @ianlancetaylor @rsc @DeedleFake @seankhliao

@gopherbot gopherbot added this to the Proposal milestone Nov 11, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 11, 2021

Change https://golang.org/cl/363374 mentions this issue: net: add missing AddrPort Listen and Dial functions for UDP and TCP

Loading

@mdlayher
Copy link
Member

@mdlayher mdlayher commented Nov 11, 2021

I am generally in favor of API symmetry between related types, but I am not sure that 5 additional constructors are justified for dial/listen operations since they are not used in allocation hot paths like the aforementioned UDP read/write methods.

It's a one liner to convert netip.AddrPorts to the appropriate type for each of the existing Dial/Listen constructors (as you've done in the CL), so is it necessary to expand the API surface of net for these minor conveniences?

Loading

@zx2c4
Copy link
Contributor Author

@zx2c4 zx2c4 commented Nov 11, 2021

I assume the goal would be to eventually mark everything having to do with UDP/TCPAddr and net.IP as deprecated, and refactor those APIs to be converters to native netip.Addr/AddrPort functions. So adding the correct API is a necessary step in getting rid of the old problematic types (or, rather, relegating them to a deprecated wrapper file for Go 1).

Loading

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Nov 11, 2021

as deprecated

Speaking of deprecated: anything without a context can be considered deprecated, so I wouldn't add anything new without context support.

We were discussing that the only API we should probably add are new methods on Dialer (ala #49097 (comment)) and ListenConfig.

Loading

@zx2c4
Copy link
Contributor Author

@zx2c4 zx2c4 commented Nov 11, 2021

Speaking of deprecated: anything without a context can be considered deprecated,

Is this documented anywhere? If the doc comments marked these as deprecated, it might motivate a lot of folks to move to context, as IDEs tend to mark deprecated function invocations with a strike or italics or some other marker.

Loading

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Nov 11, 2021

And that's exactly why we haven't deprecated them officially. There's been talk (#48665, etc) about a lighter deprecation annotation that's like, "There are better ways to do this, but this way's fine I guess."

Loading

@zx2c4
Copy link
Contributor Author

@zx2c4 zx2c4 commented Nov 11, 2021

Ah. I was thinking the strike/italics/etc as a good way to enact change without breaking the Go 1 promise, rather than a prohibitive one. But I suppose if it's still "fine", seems like moving those over to netip.Addr[Port] would be a good idea?

Loading

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Nov 11, 2021

I was thinking the strike/italics/etc as a good way

That's #16666

Loading

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants