Skip to content

Commit

Permalink
Fix MuxHandler initialization race (#1626)
Browse files Browse the repository at this point in the history
Fix race condition that can occur when calling handlers returned from
Controller.MuxHandler where the Service's slice of middleware functions
is written to in a way that is not thread-safe.

The race will only occur when the capacity of the Service's middleware
slice is greater than its length and when the number of Controller
middlewares is more than 0. The functions returned by MuxHandler (that
are all from the same Controller object), if called in parallel, could
cause the race to occur.
  • Loading branch information
rwool authored and raphael committed Mar 23, 2018
1 parent b9810fb commit cfffe9c
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 2 deletions.
2 changes: 1 addition & 1 deletion Makefile
Expand Up @@ -62,7 +62,7 @@ cyclo:
fi

test:
@ginkgo -r --randomizeAllSpecs --failOnPending --randomizeSuites -skipPackage vendor
@ginkgo -r --randomizeAllSpecs --failOnPending --randomizeSuites -race -skipPackage vendor
go test ./_integration_tests

goagen:
Expand Down
3 changes: 2 additions & 1 deletion service.go
Expand Up @@ -310,7 +310,8 @@ func (ctrl *Controller) MuxHandler(name string, hdlr Handler, unm Unmarshaler) M
}
return nil
}
chain := append(ctrl.Service.middleware, ctrl.middleware...)
mwLen := len(ctrl.Service.middleware)
chain := append(ctrl.Service.middleware[:mwLen:mwLen], ctrl.middleware...)
ml := len(chain)
for i := range chain {
handler = chain[ml-i-1](handler)
Expand Down
44 changes: 44 additions & 0 deletions service_test.go
Expand Up @@ -11,6 +11,8 @@ import (

"context"

"sync"

"github.com/goadesign/goa"
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
Expand Down Expand Up @@ -185,6 +187,48 @@ var _ = Describe("Service", func() {
Ω(muxHandler).ShouldNot(BeNil())
})

Context("with multiple instances and middlewares", func() {
var ctrl *goa.Controller
var handlers []goa.MuxHandler
var rws []*TestResponseWriter
var reqs []*http.Request
var p url.Values
var wg sync.WaitGroup

BeforeEach(func() {
nopHandler := func(context.Context, http.ResponseWriter, *http.Request) error {
return nil
}
ctrl = s.NewController("test")
for i := 0; i < 5; i++ {
ctrl.Service.Use(func(goa.Handler) goa.Handler {
return nopHandler
})
}
ctrl.Use(func(goa.Handler) goa.Handler { return nopHandler })
for i := 0; i < 10; i++ {
tmp := ctrl.MuxHandler("test", nopHandler, nil)
handlers = append(handlers, tmp)
rws = append(rws, &TestResponseWriter{})
req, _ := http.NewRequest("GET", "", nil)
reqs = append(reqs, req)
}
p = url.Values{}
wg = sync.WaitGroup{}
wg.Add(10)
})

It("doesn't race with parallel handler calls", func() {
for i := range handlers {
go func(j int) {
handlers[j](rws[j], reqs[j], p)
wg.Done()
}(i)
}
wg.Wait()
})
})

Context("with a request", func() {
var rw http.ResponseWriter
var r *http.Request
Expand Down

0 comments on commit cfffe9c

Please sign in to comment.