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.

Sign up for GitHub

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

Jump to bottom

httpGw: make /tcp/0 work #1287

Merged
merged 2 commits into from
Jun 8, 2015
Merged

httpGw: make /tcp/0 work #1287

merged 2 commits into from
Jun 8, 2015

Conversation

cryptix
Copy link
Contributor

@cryptix cryptix commented May 23, 2015

Prints out the chosen port and puts it into the config so the cli works over it too.

Fixes #1288

@jbenet jbenet added the status/in-progress In progress label May 23, 2015
cryptix
cryptix reviewed May 23, 2015
View reviewed changes
core/corehttp/corehttp.go
@@ -67,6 +68,17 @@ func listenAndServe(node *core.IpfsNode, addr ma.Multiaddr, handler http.Handler
return err
}

host, port, err := net.SplitHostPort(list.Addr().String())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there a helper somewhere to construct a multiaddr form a net.Addr directly? found it..

@krl krl mentioned this pull request May 24, 2015
Closed
@cryptix
Copy link
Contributor Author

cryptix commented May 24, 2015

Still WIP, After talking with @krl and @jbenet, we want to see if we can put the address in the repo.lock.

Would require centralizing the net.Listen() calls in one place and adding a new corehttp config option.

@whyrusleeping whyrusleeping mentioned this pull request May 26, 2015
Closed
49 tasks
jbenet
jbenet reviewed May 28, 2015
View reviewed changes
cmd/ipfs/daemon.go
if err != nil {
res.SetError(err, cmds.ErrNormal)
return
}
Copy link
Member

Choose a reason for hiding this comment

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

sure it's ok to move this check down? not sure if it was up above for a reason (i.e. bail out early before something else)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hrm.. that's a good point - my idea was to group the bring-up of fuse mount, gateway and API. I can revert it if you prefer the early-error out. I'm not sure if daemon startup failure something you want to optimizer over clarity for though.

Copy link
Member

Choose a reason for hiding this comment

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

agree with you. if there's no error here, let's move it down.

@cryptix
Copy link
Contributor Author

cryptix commented May 28, 2015

rebased

@cryptix
Copy link
Contributor Author

cryptix commented May 29, 2015

rebased (2)

@jbenet
Copy link
Member

jbenet commented May 29, 2015

Running tests on all commits

@cryptix cryptix force-pushed the feat/httpApiTcpPort0 branch 2 times, most recently from 9a2bc59 to d8c11a5 Compare May 30, 2015 00:34
@cryptix
Copy link
Contributor Author

cryptix commented May 30, 2015

squashed and rebased onto master.

i reorderd it to do api first, gateway 2nd and had to change the way corehttp.Serve errors are gathered. I like this more though, gateway http errors were only logged and not returend.

@jbenet
Copy link
Member

jbenet commented May 30, 2015

@cryptix would you mind splitting this into function for the steps? one big function is scary to edit because not clear what is required to be in that order. functions are nicer because smaller units of work are easier to reason about. you have this paged into your mind right now and are best suited for it.

@cryptix cryptix force-pushed the feat/httpApiTcpPort0 branch 2 times, most recently from fef207b to 2d87eaa Compare May 31, 2015 12:11
@cryptix
Copy link
Contributor Author

cryptix commented May 31, 2015

rebased for fun and test results

@cryptix cryptix force-pushed the feat/httpApiTcpPort0 branch 2 times, most recently from 563760b to cea5aa2 Compare June 1, 2015 14:30
@cryptix
Copy link
Contributor Author

cryptix commented Jun 1, 2015

@jbenet i reworked the helper to not goroutine until necessary, this way they don't race on writing to stdout (this caused the weird 'daemon output not okay' test faults i belive)

RFM if its fine with you.

whyrusleeping
whyrusleeping reviewed Jun 1, 2015
View reviewed changes
cmd/ipfs/daemon.go
if len(cfg.Gateway.RootRedirect) > 0 {
rootRedirect = corehttp.RedirectOption("", cfg.Gateway.RootRedirect)
// mountHTTPapi collects options, creates listener, prints status message and starts serving requests
func mountHTTPapi(req cmds.Request) (error, <-chan error) {
Copy link
Member

Choose a reason for hiding this comment

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

calling it mount might confuse things with fuse

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm.. yea.. open or listenAndServe?

Copy link
Member

Choose a reason for hiding this comment

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

maybe serveHTTPApi

@whyrusleeping whyrusleeping mentioned this pull request Jun 2, 2015
Closed
58 tasks
jbenet
jbenet reviewed Jun 3, 2015
View reviewed changes
cmd/ipfs/daemon.go
}

// mountHTTPgw collects options, creates listener, prints status message and starts serving requests
func mountHTTPgw(req cmds.Request) (error, <-chan error) {
Copy link
Member

Choose a reason for hiding this comment

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

serveHTTPGateway

Copy link
Member

Choose a reason for hiding this comment

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

(should it be serveHttpGateway? dont recall what's go idiomatic)

@jbenet
Copy link
Member

jbenet commented Jun 3, 2015

@cryptix more comments o/

@cryptix
http endpoints: dont print before listen
07b3415 
also splits api, gw and fuse bring up into helper functions
@cryptix
daemon: addressed CR comments
4b337bb
@cryptix
Copy link
Contributor Author

cryptix commented Jun 8, 2015

rebased and addressed comments

@jbenet
Copy link
Member

jbenet commented Jun 8, 2015

LGTM i'll merge after tests run

jbenet added a commit that referenced this pull request Jun 8, 2015
@jbenet
Merge pull request #1287 from ipfs/feat/httpApiTcpPort0
217fe26 
httpGw: make /tcp/0 work
@jbenet jbenet merged commit 217fe26 into master Jun 8, 2015
@jbenet jbenet removed the status/in-progress In progress label Jun 8, 2015
@jbenet jbenet deleted the feat/httpApiTcpPort0 branch June 8, 2015 10:46
@whyrusleeping whyrusleeping mentioned this pull request Jun 9, 2015
Closed
50 tasks
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
@jbenet
Merge pull request ipfs/kubo#1287 from ipfs/feat/httpApiTcpPort0
94f14a6 
httpGw: make /tcp/0 work

This commit was moved from ipfs/kubo@217fe26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ipfs prints API server listening before actually opening a port
3 participants
@cryptix @jbenet @whyrusleeping