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

p2p/net/conn: reuse listen tcp port to dial #602

Merged
merged 9 commits into from
Jan 20, 2015
Merged

p2p/net/conn: reuse listen tcp port to dial #602

merged 9 commits into from
Jan 20, 2015

Conversation

jbenet
Copy link
Member

@jbenet jbenet commented Jan 19, 2015

This PR finally makes it possible to dial out from our TCP listen addr. I had to sink low into net to make this possible. The result is the go-reuseport package. Sadly golang/net doesnt let us add tcp socket options, so we have to do it this way (setting up the sockets manually, which is truly painful). can see the same approach used in various modules:

Anyway, this PR:

  • adds go-reuseport (and its dep, go-sockaddr/net)
  • changes p2p/net/conn to optimistically use go-reuseport (it can't always work because we may be dialing the same 5-tuple), or fallback to reg dial.
  • changed the identify receiver to register the observed address, if and only if it is for a connection whose LocalAddress() is one of our ListenAddrs().

[1] wow i just discovered this one... after rolling my own. fffff! it looks good-- we might want to switch to this one if my go-reuseport gives us problems

@jbenet
Copy link
Member Author

jbenet commented Jan 19, 2015

https://build.protocol-dev.com/job/ipfs_test_all_commits_on_branch/185/console <-- tells me tests_all_commits fails, but i cant tell which commit. make test_all_commits passes locally

@jbenet
Copy link
Member Author

jbenet commented Jan 19, 2015

RFCR

@whyrusleeping
Copy link
Member

this PR shouldnt be against ipfs, you should file it against Go.

return -1, err
}

// set non-blocking until after connect, because we cant poll using runtime :(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whats this?

@whyrusleeping
Copy link
Member

Is much of this code directly copied from go's net lib? If so, it might be good to mark any of that as such.

@whyrusleeping whyrusleeping added this to the α milestone Jan 19, 2015
@jbenet
Copy link
Member Author

jbenet commented Jan 19, 2015

this PR shouldnt be against ipfs, you should file it against Go.

Right!?

Is much of this code directly copied from go's net lib? If so, it might be good to mark any of that as such.

Yeah, the stuff copied straight out is marked with the copyright notice. The connect() function is a stripped down version of theirs.

@jbenet
Copy link
Member Author

jbenet commented Jan 20, 2015

This amount of copy pasting is horrible. This PR is an exercise in why internal packages / forcibly unaccessible symbols are a bad idea. There's a significant difference between "not exporting something to my interface" and "making it absolutely impossible for you to use this code". It should be possible to import things if you really mean it. Maybe as a different package altogether with a dependency like ibaz "foo/bar/baz/internal".

@jbenet
Copy link
Member Author

jbenet commented Jan 20, 2015

https://build.protocol-dev.com/job/build%20docker%20image/923/console

Sending build context to Docker daemon 
Step 0 : FROM golang:1.3
 ---> ebd3fd90ae2e
Step 1 : MAINTAINER Brian Tiger Chow <btc@perfmode.com>
 ---> Using cache
 ---> 6c893fd2f62d
Step 2 : ADD . /go/src/github.com/jbenet/go-ipfs
 ---> a18697f6bce6
Removing intermediate container 764cfa2d8ed1
Step 3 : RUN cd /go/src/github.com/jbenet/go-ipfs/cmd/ipfs && go install
 ---> Running in 3452c2eb3913
 ---> 89b1045e2b93
Removing intermediate container 3452c2eb3913
Step 4 : EXPOSE 4001 5001 4002/udp
 ---> Running in ee9c69101f50
 ---> 2c6dc0ce7374
Removing intermediate container ee9c69101f50
Step 5 : ENTRYPOINT ipfs
 ---> Running in b5429cafd23b
2015/01/20 00:14:49 An error occured while creating the container
Build step 'Execute shell' marked build as failure
Warning: you have no plugins providing access control for builds, so falling back to legacy behavior of permitting any downstream builds to be triggered
Triggering a new build of chore_reclaim-docker-space
Finished: FAILURE

Maybe the chore should happen before the docker image tries to build and fails? or this may be unrelated.

cc @briantigerchow

@whyrusleeping
Copy link
Member

Looks pretty good to me, let's hope it solves our issues!

@jbenet
Copy link
Member Author

jbenet commented Jan 20, 2015

@whyrusleeping is that a LGTM on the PR?

This commit finally makes use of the sent observed addrs.
If the connection's local address is from one of our
listen addrs, then the remote's observed addr is its
natted mapping, which is useful to us. For now, we add
it directly to our address book. (a future commit should
make addressbook addresses expire)
This commit cleans up the reuse port setup, and fixes a problem:
make sure to filter addrs out that we simply cannot dial with
(e.g. loopback -> non-loopback, or linklocal -> nonlinklocal)
@whyrusleeping
Copy link
Member

Yeah, merge it! (before i change my mind! :P )

jbenet added a commit that referenced this pull request Jan 20, 2015
p2p/net/conn: reuse listen tcp port to dial
@jbenet jbenet merged commit 97333ae into master Jan 20, 2015
@jbenet jbenet removed the status/in-progress In progress label Jan 20, 2015
@jbenet jbenet deleted the reuseport branch January 20, 2015 07:38
@jbenet jbenet mentioned this pull request Jan 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants