search

anacrolix / utp

Use anacrolix/go-libutp instead
https://github.com/anacrolix/go-libutp
Mozilla Public License 2.0
173 stars 35 forks source link

Use real address of remote host. #18

Closed mjgarton closed 7 years ago

mjgarton commented 7 years ago

Use actual address used, instead of address passed by caller. This means we can dialling 0.0.0.0 and it will work, similar to how TCP would work.

prettymuchbryce commented 7 years ago

Hmmm. Does it make sense to conditionally check if the addr in newConn is an ipv4 or ipv6 address matching the form 0.0.0.0 or [::] respectively ? We might not want to assume that the RemoteAddr() is the intended recipient for every case, and we could also avoid doing the extra dial this way.

Also it looks like you may want to change some references from netAddr.String() to c.RemoteAddr() in the print statements of DialTimeout.

mjgarton commented 7 years ago

I addressed some of your concerns. Still thinking about the other part.

anacrolix commented 7 years ago

I think I'm experiencing the XY problem here. What are you trying to achieve?

mjgarton commented 7 years ago

I am trying to make UTP work better as a drop in replacement for TCP in some contexts. For example, I can Listen on 0.0.0.0:1234, and then Dial 0.0.0.0:1234 with TCP, but that doesn't currently work with this UTP implementation. My change fixes that.

anacrolix commented 7 years ago

Why can't you Dial localhost:1234, or the interface specific IP:1234? Something tells me dialling 0.0.0.0 is going to be OS specific.

mjgarton commented 7 years ago

I could do that, but I'm trying to make the behaviour as similar as I can to how TCP works, so utp can be used as a drop-in replacement without other modifications. I can't see why utp should be different from TCP in that sense.

As for being OS specific, it's true that 0.0.0.0 hasn't worked on Windows so far, but that's considered a bug and it being fixed for go 1.8 I believe.

mjgarton commented 7 years ago

FYI here is some work towards making 0.0.0.0 work in the standard library. https://github.com/golang/go/commit/1a0b1cca4c26d41fe7508ffdb355de78b4ea2a19

If it helps, I think my code can be simplified to check specifically for 0.0.0.0 and not use the UDP Dial approach I used.

prettymuchbryce commented 7 years ago

@mjgarton Based on the commit you linked it looks like this change to ipsock_posix.go should eventually solve the problem for Windows. It looks to me like going forward 0.0.0.0 will be substituted with 127.0.0.1 automatically by the socket primitives in the Go standard library for the systems which don't already implement 0.0.0.0 as a loopback address. I don't think there is any requirement for the OS to implement 0.0.0.0 as a loopback address.

There is some mention of it in RFC1122:

(a) { 0, 0 } This host on this network. MUST NOT be sent, except as a source address as part of an initialization procedure by which the host learns its own IP address.

One point of confusion for me is that you mention mirroring TCP behavior, but anecdotally it looks like to me on Mac OSX that sending a single UDP packet to 0.0.0.0 does implement the loopback as well. I think ultimately this may boil down to a decision around whether this behavior should be mirrored for utp.

mjgarton commented 7 years ago

@prettymuchbryce My reference to the commit was just to illustrate that there seems to be an intention to make this work cross platform in the standard library (and actually of course it's for UDP and not just TCP). It already worked before that change on Linux at least, and on Mac OSX.

Ultimately, I agree with your point that it comes down to whether we want to mirror this for utp, but since utp is build upon UDP, and UDP packets sent from 0.0.0.0 actually end up with source address 127.0.0.1 on them, my change pretty much just acknowledges that fact and allows the utp implementation to keep track of connections properly.

Perhaps I should have simply checked for 0.0.0.0 specifically and replaced with 127.0.0.1 though, rather than doing the UDP send trick to discover the source address.

anacrolix commented 7 years ago

Thanks for the effort so far. If indeed dialing 0.0.0.0 works for TCP and UDP on some platforms, but not for utp, then it is desirable to fix those cases. We should have test cases that check that utp's Conn.RemoteAddr/Conn.LocalAddr/Dial/Accept mirror their TCP counterparts. I don't think dummy Dials are the right way to do it. Insert address substitutions per the changes you linked to being made in Go if necessary.

mjgarton commented 7 years ago

I made an alternative PR that is quite different in implementation, but I think closer to what we are beginning to agree on. https://github.com/anacrolix/utp/pull/19

anacrolix commented 7 years ago

Good work. Appreciated