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: Addr, LocalAddr, RemoteAddr must return established endpoint addresses #9654

Open
keks opened this Issue Jan 21, 2015 · 6 comments

Comments

Projects
None yet
6 participants
@keks

keks commented Jan 21, 2015

Example here: http://play.golang.org/p/0zfs2UZ7ZY

I don't know if this is intended, but it feels weird.

@keks keks changed the title from UDPConn.LocalAddr().IP can be changed and the next call will return the changed value to The value returned by UDPConn.LocalAddr().IP can be changed and the next call will return the changed value Jan 21, 2015

@mikioh mikioh changed the title from The value returned by UDPConn.LocalAddr().IP can be changed and the next call will return the changed value to net: the returned value from {Remote,Local}Addr of {TCP,UDP,IP,Unix}Conn is not immutable Jan 21, 2015

@mikioh mikioh changed the title from net: the returned value from {Remote,Local}Addr of {TCP,UDP,IP,Unix}Conn is not immutable to net: the returned value from Addr, LocalAddr, RemoteAddr on every connection type is not immutable Jan 22, 2015

@keks

This comment has been minimized.

keks commented Feb 1, 2015

I think the obvious behaviour would be to give the caller a copy of the Addr. After looking at the code I think the only way to do this is to give Addr the ability to copy itself. However, we can't change the Addr interface, so I think introducing an interface like

type AddrCopier interface{
  func Copy() Addr
}

might work around the problem. Make all the net.(.*)Addr types implement this function and let LocalAddr() (and probably RemoteAddr() as well) check if the given Addr also implements AddrCopier. If so, return a copy. If not, return the original value, because that's all we got.

I know this isn't a real fix, but I think it's the only solution.

@minux

This comment has been minimized.

Member

minux commented Feb 1, 2015

I don't think this could be done: it's still a breaking change.

For example, user might use the result from LocalAddr() to index a map
that depended on the fact each invocation of the method on the same
connections returns pointer to the same Addr struct.

And I don't think this issue is a problem at all. Go doesn't have immutable
types, so if you modify types returned from any method, you'd better
know what you're doing.

@mikioh mikioh changed the title from net: the returned value from Addr, LocalAddr, RemoteAddr on every connection type is not immutable to net: Addr, LocalAddr, RemoteAddr must return established endpoint addresses Feb 1, 2015

@mikioh

This comment has been minimized.

Contributor

mikioh commented Feb 1, 2015

@keks,

Please open a new issue if you have something for the immutability of concrete types that implement net.Addr interface. This issue should focus on the behavior of Addr, LocalAddr and RemteAddr.

@mikioh

This comment has been minimized.

Contributor

mikioh commented Feb 1, 2015

Go doesn't have immutable types

True, except string types.

so if you modify types returned from any method, you'd better know what you're doing.

Like other APIs such as various DNS record lookups, listing nework interfaces and addresses, re-using the returned values from the net package is fine. I think this issue has been overlooked for a long time.

@mikioh mikioh self-assigned this Feb 1, 2015

@mikioh mikioh added the Go2 label Feb 3, 2015

@minux

This comment has been minimized.

Member

minux commented Feb 4, 2015

In Go 2, we probably should make all existing net.Addr implementations implement
the net.Addr methods on non-pointer receiver, so that Addr, LocalAddr, RemoteAddr
could return a copy of the Addr.

This is in line with other packages that returns internal state that shouldn't be modified.

@keks

This comment has been minimized.

keks commented Feb 5, 2015

Alright, I think this is reasonable. I don't see an elegant way of doing this without non-pointer receivers.

minux added a commit that referenced this issue Feb 6, 2015

net: document that user shouldn't modify returned Addr
Ideally, those methods should return a copy of the Addr, but
due to the Go 1 API guarantee, we cannot make that change now:
there might exist client code that uses the returned Addr as
map index and thus relies on the fact that different invocation
of the method returns the same pointer. Changing this behavior
will lead to hidden behaviour change in those programs.

Update #9654.

Change-Id: Iad4235f2ed7789b3a3c8e0993b9718cf0534ea2b
Reviewed-on: https://go-review.googlesource.com/3851
Reviewed-by: Mikio Hara <mikioh.mikioh@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

@rsc rsc added this to the Unplanned milestone Apr 10, 2015

@mikioh mikioh removed their assignment May 21, 2015

@rsc rsc changed the title from net: Addr, LocalAddr, RemoteAddr must return established endpoint addresses to proposal: net: Addr, LocalAddr, RemoteAddr must return established endpoint addresses Jun 17, 2017

@gopherbot gopherbot added the Proposal label Jun 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment