Skip to content

Commit

Permalink
fix: plumb context into network methods
Browse files Browse the repository at this point in the history
so, turns out cmd package tests that ran after an rpc server starts were all running over rpc, which is bad. This makes that not bad by just fixing the problem, passing in a context that cancels with the test. Its a change we need to make anyway, just not one I thought we'd need to make this go-round. programming is hard.
  • Loading branch information
b5 committed May 9, 2019
1 parent 1ea6a43 commit fb036e3
Show file tree
Hide file tree
Showing 12 changed files with 152 additions and 58 deletions.
24 changes: 19 additions & 5 deletions api/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"context"
"fmt"
"io"
"io/ioutil"
stdlog "log"
"net"
"net/http"
"net/rpc"
Expand All @@ -29,7 +31,7 @@ func init() {
// a few methods don't conform to the proper signature (comment this out & run 'qri connect' to see errors)
// so we're disabling the log package for now. This is potentially very stupid.
// TODO (b5): remove dep on net/rpc package entirely
// stdlog.SetOutput(ioutil.Discard)
stdlog.SetOutput(ioutil.Discard)

golog.SetLogLevel("qriapi", "info")
}
Expand All @@ -46,7 +48,7 @@ func New(inst *lib.Instance) (s Server) {
}

// Serve starts the server. It will block while the server is running
func (s Server) Serve() (err error) {
func (s Server) Serve(ctx context.Context) (err error) {
node := s.Node()
cfg := s.Config()

Expand All @@ -59,8 +61,8 @@ func (s Server) Serve() (err error) {
mux := NewServerRoutes(s)
server.Handler = mux

go s.ServeRPC()
go s.ServeWebapp()
go s.ServeRPC(ctx)
go s.ServeWebapp(ctx)

if namesys, err := node.GetIPFSNamesys(); err == nil {
if pinner, ok := node.Repo.Store().(cafs.Pinner); ok {
Expand Down Expand Up @@ -122,12 +124,18 @@ func (s Server) Serve() (err error) {
}(server, cfg.API.DisconnectAfter)
}

go func() {
<-ctx.Done()
log.Info("shutting down")
server.Close()
}()

// http.ListenAndServe will not return unless there's an error
return StartServer(cfg.API, server)
}

// ServeRPC checks for a configured RPC port, and registers a listner if so
func (s Server) ServeRPC() {
func (s Server) ServeRPC(ctx context.Context) {
cfg := s.Config()
if !cfg.RPC.Enabled || cfg.RPC.Port == 0 {
return
Expand All @@ -146,6 +154,12 @@ func (s Server) ServeRPC() {
}
}

go func() {
<-ctx.Done()
log.Info("closing RPC")
listener.Close()
}()

rpc.Accept(listener)
return
}
Expand Down
9 changes: 8 additions & 1 deletion api/webapp.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
)

// ServeWebapp launches a webapp server on s.cfg.Webapp.Port
func (s Server) ServeWebapp() {
func (s Server) ServeWebapp(ctx context.Context) {
cfg := s.Config()
if !cfg.Webapp.Enabled || cfg.Webapp.Port == 0 {
return
Expand All @@ -26,6 +26,13 @@ func (s Server) ServeWebapp() {
m.Handle("/webapp/", s.FrontendHandler("/webapp"))

webappserver := &http.Server{Handler: m}

go func() {
<-ctx.Done()
log.Info("clsing webapp server")
webappserver.Close()
}()

webappserver.Serve(listener)
return
}
Expand Down
7 changes: 5 additions & 2 deletions api/webapp_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package api

import (
"context"
"fmt"
"io/ioutil"
"net/http"
Expand All @@ -11,19 +12,21 @@ import (
)

func TestServeWebapp(t *testing.T) {
ctx, done := context.WithCancel(context.Background())
defer done()

node, teardown := newTestNode(t)
defer teardown()
cfg := config.DefaultConfigForTesting()
cfg.Webapp.Enabled = false
// TODO (b5) - hack until tests have better instance-generation primitives
inst := lib.NewInstanceFromConfigAndNode(cfg, node)
New(inst).ServeWebapp()
New(inst).ServeWebapp(ctx)

cfg.Webapp.EntrypointUpdateAddress = ""
cfg.Webapp.Enabled = true
s := New(inst)
go s.ServeWebapp()
go s.ServeWebapp(ctx)

url := fmt.Sprintf("http://localhost:%d", cfg.Webapp.Port)
res, err := http.Get(url)
Expand Down
6 changes: 5 additions & 1 deletion cmd/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
package cmd

import (
"context"
"fmt"
"io"
"os"
Expand Down Expand Up @@ -42,7 +43,10 @@ func Execute() {

ensureLargeNumOpenFiles()

root := NewQriCommand(EnvPathFactory, gen.NewCryptoSource(), ioes.NewStdIOStreams())
// root context
ctx := context.Background()

root := NewQriCommand(ctx, EnvPathFactory, gen.NewCryptoSource(), ioes.NewStdIOStreams())
// If the subcommand hits an error, don't show usage or the error, since we'll show
// the error message below, on our own. Usage is still shown if the subcommand
// is missing command-line arguments.
Expand Down
Loading

0 comments on commit fb036e3

Please sign in to comment.