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

Closed
zx2c4 opened this issue Nov 11, 2021 · 18 comments
Closed

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

zx2c4 opened this issue Nov 11, 2021 · 18 comments

Comments

@zx2c4
Copy link
Contributor

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

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

@mdlayher
Copy link
Member

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?

@zx2c4
Copy link
Contributor Author

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).

@bradfitz
Copy link
Contributor

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.

@zx2c4
Copy link
Contributor Author

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.

@bradfitz
Copy link
Contributor

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."

@zx2c4
Copy link
Contributor Author

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?

@bradfitz
Copy link
Contributor

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

That's #16666

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Nov 11, 2021
@rsc
Copy link
Contributor

rsc commented Dec 1, 2021

I don't think we should deprecate net.DialTCP. Deprecate means IDEs and such will warn about usage, and that's unnecessary.

Let's redirect this conversation to #49097. Closing as duplicate of that.

@zx2c4
Copy link
Contributor Author

zx2c4 commented Dec 1, 2021

Before you close this issue, there's still https://go-review.googlesource.com/c/go/+/363374/ to consider, right?

@rsc
Copy link
Contributor

rsc commented Dec 1, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals (old) Dec 1, 2021
@rsc
Copy link
Contributor

rsc commented Dec 8, 2021

@zx2c4, clearly I forgot to close this issue last week. CL 363374 implements this proposal, but I think we want to instead add the new functions in Dialer only. Using the typed forms is pretty rare for non-UDP things. I will leave this open to let this discussion about the non-Dialer functions continue toward a consensus, but it seems like we should do just the Dialer changes and not do CL 363374.

@rsc
Copy link
Contributor

rsc commented Dec 15, 2021

Does anyone object to only adding new Dialer methods, and not adding any top-level non-Dialer functions for AddrPort?

@zx2c4
Copy link
Contributor Author

zx2c4 commented Dec 15, 2021

I'm probably a lone voice out there, so feel free to disregard that, but my feeling is that if we're moving to a new type, we should have API parity, and I sort of like the convenience of the top level functions for one-off little scripts and stuff. I don't really feel so strongly about it, though, and I recognize I'm likely in the minority, so 🤷 .

@rsc
Copy link
Contributor

rsc commented Jan 5, 2022

These functions are so little used that the API parity may not be worthwhile.
It sounds like there aren't too many objections to only adding new Dialer methods, so we should probably start with that.
We can always come back and add the others later if the need becomes clearer.

@rsc
Copy link
Contributor

rsc commented Jan 5, 2022

This will become a likely decline, and the Dialer methods are #49097.

@rsc
Copy link
Contributor

rsc commented Jan 5, 2022

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Jan 5, 2022
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Jan 12, 2022
@rsc
Copy link
Contributor

rsc commented Jan 12, 2022

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc closed this as completed Jan 12, 2022
@golang golang locked and limited conversation to collaborators Jan 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

5 participants