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

net/netip: missing odds and ends for 1.18 [freeze exception] #49298

Closed
zx2c4 opened this issue Nov 2, 2021 · 23 comments
Closed

net/netip: missing odds and ends for 1.18 [freeze exception] #49298

zx2c4 opened this issue Nov 2, 2021 · 23 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@zx2c4
Copy link
Contributor

zx2c4 commented Nov 2, 2021

Now that netip has landed, I've started porting code to it. I'm hoping to get a bit of experience doing this before Go 1.18 in order to catch issues before it ships. So far I've identified six things that are missing, which I was hoping we could get into Go 1.18, as a followup to #46518.

Items with ⏳ have CLs still pending, and items with ✅ have CLs that have been submitted.


1. MarshalBinary/UnmarshalBinary on AddrPort and Prefix

Addr.MarshalBinary and Addr.UnmarshalBinary both exist. However, the same is not true for the remaining two types, AddrPort and Prefix. This makes it very hard to use with encoding/gob, because not only are these marshals missing, but the inner addr + port/cidr members are unexported. So, I propose adding these two methods, with implementations that simply concatenate Addr.MarshalBinary output with little-endian encoding of the port (uint16) for AddrPort or cidr (uint8) for Prefix.

2. Addr.AsSlice

There is netip.AddrFromSlice() and netip.AddrFrom4, but there is only netip.As4 and no netip.AsSlice. This may seem kind of trifling, but it winds up being kind of annoying when coding things; I find myself writing a lot of:

var s []byte
if addr.Is4() {
    arr := addr.As4()
    s = arr[:]
} else addr.Is16() {
    arr := addr.As16()
    s = arr[:]
}

You can't slice an rvalue, so that temporary is needed. And sometimes I just want a slice, regardless of what kind of IP it is, and having to open code dispatch for that every time is a bit of a pain. The above snippet would be simplified to:

s := addr.AsSlice()

3. IPv4Unspecified()

There is IPv6Unspecified(), but not IPv4Unspecified(). This is sort of confusing, and code would be more clear and consistent if I could write netip.IPv4Unspecified() instead of netip.AddrFrom4([4]byte{}).

4. One remaining allocation in UDP path

The "net" package has one remaining allocation in the UDP packet path, which @josharian has a change to fix, so that UDP sends/receives can be allocation-free. Being alloc-free is one of the original motivations behind netip in the first place.

5. Missing UDPAddr and TCPAddr analogs ❌ (partially accepted, partially punted to go 1.19)

We have ReadFromUDPAddrPort and such, for reads and writes of UDP and TCP sockets, but the listener/dialer functions still take the old TCPAddr and UDPAddr structs. So, we'll need new functions there, to avoid unwanted type conversions by users.

6. AddrFromSlice should return only one value ❌ (rejected)

The AddrFrom4 and AddrFrom6 functions never fail, and that makes it easy to pass them around with function composition. AddrFromSlice, however, awkwardly returns an ok value. But, when ok==false, the returned Addr isn't valid anyway. So folks who want to check for validity can just do addr.IsValid() like normal, instead of having to learn something new and odd. So, we should drop the ok return value, especially as most users already ignore it.


(I'll update this issue if I find anything new.)

CC @bradfitz @crawshaw @josharian @danderson @ianlancetaylor

@zx2c4 zx2c4 changed the title net/netip: missing odds and ends for 1.18 proposal: net/netip: missing odds and ends for 1.18 Nov 2, 2021
@gopherbot gopherbot added this to the Proposal milestone Nov 2, 2021
@zx2c4
Copy link
Contributor Author

zx2c4 commented Nov 2, 2021

golang.zx2c4.com/go118/netip contains an extraction of Go 1.18's netip, so folks can experiment with this before Go 1.18 ships.

import "golang.zx2c4.com/go118/netip"

@bradfitz
Copy link
Contributor

bradfitz commented Nov 2, 2021

(1) is just an omission. I think @josharian was going to send a change for that.

(2) doesn't seem objectionable to me at least. We had that originally as https://pkg.go.dev/inet.af/netaddr#IP.IPAddr but for circular dependency reasons we couldn't include it in net/netip.

@rsc, opinions?

@dave-andersen
Copy link
Contributor

(psst, I think you may have meant to tag @danderson instead of me. :-)

@gopherbot
Copy link

Change https://golang.org/cl/360874 mentions this issue: net/netip: add Addr.AsSlice() method

@DeedleFake
Copy link

Unless I'm misunderstanding something, AsSlice()'s functionality seems to already be covered by IPAddrParts(), MarshalBinary(), and the unexported ipZone().

@seankhliao
Copy link
Member

Maybe we can drop IPAddrParts()? the discoverability of AsSlice() is better and there's already a Zone().
MarshalBinary() also encodes the zone, so it's something different

@gopherbot
Copy link

Change https://golang.org/cl/360875 mentions this issue: net/netip: add missing encoding.BinaryUnmarshaler to AddrPort and Prefix

@zx2c4
Copy link
Contributor Author

zx2c4 commented Nov 2, 2021

The idea of AsSlice would be to return a single return value, like As4 and As6, so that it can easily and ergonomically be passed to functions and such.

The CL has been updated with @seankhliao's suggestion.

@zx2c4
Copy link
Contributor Author

zx2c4 commented Nov 3, 2021

Top post of issue updated to contain mention of IPv4Unspecified.

@gopherbot
Copy link

Change https://golang.org/cl/361056 mentions this issue: net/netip: add IPv4Unspecified

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

bradfitz commented Nov 3, 2021

Also, @josharian has a change to remove the remaining 1 alloc in the UDP paths so UDP sends/receives can be allocation-free, which is the main motivation of netip in the first place. It'd be a better story if the package & its benefit came together in the same release, rather than waiting for Go 1.19 to make use of netip in net be zero-alloc.

In the same spirit of the #49073 freeze exception proposal, this is a proposal that we finish the misc loose ends on netip for 1.18.

@bradfitz bradfitz changed the title proposal: net/netip: missing odds and ends for 1.18 proposal: net/netip: missing odds and ends for 1.18 [freeze exception] Nov 3, 2021
@rsc rsc changed the title proposal: net/netip: missing odds and ends for 1.18 [freeze exception] net/netip: missing odds and ends for 1.18 [freeze exception] Nov 3, 2021
@rsc rsc removed the Proposal label Nov 3, 2021
@rsc rsc removed this from Incoming in Proposals (old) Nov 3, 2021
@rsc rsc modified the milestones: Proposal, Go1.18 Nov 3, 2021
@ianlancetaylor
Copy link
Contributor

CC @golang/release

@dmitshur dmitshur added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 3, 2021
@zx2c4
Copy link
Contributor Author

zx2c4 commented Nov 3, 2021

Also, @josharian has a change to remove the remaining 1 alloc in the UDP paths so UDP sends/receives can be allocation-free

I've now updated the top post of this issue to include that.

@zx2c4
Copy link
Contributor Author

zx2c4 commented Nov 5, 2021

Added to top post entry about missing UDPAddr and TCPAddr analogs.

@gopherbot
Copy link

Change https://golang.org/cl/361477 mentions this issue: net: add missing AddrPort functions to TCP and UDP

@zx2c4
Copy link
Contributor Author

zx2c4 commented Nov 5, 2021

Added to top post entry about AddrFromSlice should return only one value.

@gopherbot
Copy link

Change https://golang.org/cl/361479 mentions this issue: net/netip: drop 'ok' argument in AddrFromSlice

@dmitshur
Copy link
Contributor

dmitshur commented Nov 5, 2021

We've discussed this on the release team. Freeze exception granted. Please be mindful of timing and stability as you proceed.

(Thanks for already documenting this new package in 1.18 release notes, it helps a lot!)

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Nov 5, 2021
@zx2c4
Copy link
Contributor Author

zx2c4 commented Nov 5, 2021

Great. There are currently CLs submitted for 5/6 of the items (afaik). The relation chain begins here: https://go-review.googlesource.com/c/go/+/360874/

gopherbot pushed a commit that referenced this issue Nov 5, 2021
We have AddrFrom4, AddrFrom6, AddrFromSlice and As4, As6, but we are
missing AsSlice, so this commit adds the missing function. It also gets
rid of the less ergonomic and inconsistently named IPAddrParts.

Updates #49298.

Change-Id: I1c6a2c32fc6c69b244ab49765412ffe3bbe7e5c2
Reviewed-on: https://go-review.googlesource.com/c/go/+/360874
Trust: Jason A. Donenfeld <Jason@zx2c4.com>
Trust: Josh Bleecher Snyder <josharian@gmail.com>
Run-TryBot: Jason A. Donenfeld <Jason@zx2c4.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
gopherbot pushed a commit that referenced this issue Nov 5, 2021
The Addr type got an encoding.BinaryUnmarshaler implementation, but not
AddrPort and Prefix. This commit adds the missing implementation of that
interface to these types. It also adds two round trip tests that follow
the template of the existing one for Addr.

Updates #49298.

Change-Id: Iac633aed8aac579960815bb64d06ff3181214841
Reviewed-on: https://go-review.googlesource.com/c/go/+/360875
Trust: Jason A. Donenfeld <Jason@zx2c4.com>
Run-TryBot: Jason A. Donenfeld <Jason@zx2c4.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
gopherbot pushed a commit that referenced this issue Nov 5, 2021
There is IPv6Unspecified but there is not IPv4Unspecified, making for
inconsistent code. This commit adds the missing function.

Updates #49298.

Change-Id: Id2519b646323642f59fb1cc6ea8e335fdde16290
Reviewed-on: https://go-review.googlesource.com/c/go/+/361056
Trust: Jason A. Donenfeld <Jason@zx2c4.com>
Trust: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Jason A. Donenfeld <Jason@zx2c4.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@zx2c4
Copy link
Contributor Author

zx2c4 commented Nov 9, 2021

One thing I was hoping to reach some consensus about was item 6:


6. AddrFromSlice should return only one value ⏳

The AddrFrom4 and AddrFrom6 functions never fail, and that makes it easy to pass them around with function composition. AddrFromSlice, however, awkwardly returns an ok value. But, when ok==false, the returned Addr isn't valid anyway. So folks who want to check for validity can just do addr.IsValid() like normal, instead of having to learn something new and odd. So, we should drop the ok return value, especially as most users already ignore it.


Brad mentioned in the issue that forcing people to check the ok value was a feature, not a bug. I see it differently, as a nuisance. Thought I should ask here how others feel, or if I should abandon that CL. (The CL's diff shows the simplification in other parts of the code afforded by this improvement.)

@gopherbot
Copy link

Change https://golang.org/cl/362596 mentions this issue: net: add conversion from AddrPort to TCPAddr to complement existing inverse

@gopherbot
Copy link

Change https://golang.org/cl/362595 mentions this issue: net: add missing AddrPort functions to UDP

@gopherbot
Copy link

Change https://golang.org/cl/362597 mentions this issue: net: add AddrPort Listen and Dial functions to TCP

gopherbot pushed a commit that referenced this issue Nov 11, 2021
…nverse

We already have various member functions of TCPAddr that return an
AddrPort, but we don't have a helper function to go from a AddrPort to a
TCPAddr. UDP has this, but it was left out of TCP. This commit adds the
corresponding function.

Updates #49298.

Change-Id: I85732cf34f47c792fe13a6b4af64fd4b0e85d06a
Reviewed-on: https://go-review.googlesource.com/c/go/+/362596
Trust: Jason A. Donenfeld <Jason@zx2c4.com>
Run-TryBot: Jason A. Donenfeld <Jason@zx2c4.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@zx2c4 zx2c4 closed this as completed Nov 11, 2021
@golang golang locked and limited conversation to collaborators Nov 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

9 participants