golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
124.42k stars 17.71k forks source link

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

Closed zx2c4 closed 2 years ago

zx2c4 commented 3 years ago

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:

(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 commented 3 years ago

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

mdlayher commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/golang/go/issues/49097#issuecomment-953157835) and ListenConfig.

zx2c4 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

That's #16666

rsc commented 3 years ago

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 commented 3 years ago

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

rsc commented 3 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

zx2c4 commented 2 years ago

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 :shrug: .

rsc commented 2 years ago

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 commented 2 years ago

This will become a likely decline, and the Dialer methods are https://github.com/golang/go/issues/49097.

rsc commented 2 years ago

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

rsc commented 2 years ago

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