ant0ine
commented
7 years ago Hi @gsparx !
Thanks for your bug report! And sorry for my extremely slow response. I didn't have much time for this project in a past few months. I'm not very familiar with the go test -race, but it looks very useful. And I did run your test and reproduced the output.
But I noticed that your test is instantiating the API handler twice. One in each go routine. I think this is where the problem is. A single API object with a single stack of middlewares, but with MakeHandler() called multiple times in a concurrent way.
The middlewares are "initialized" by calling MiddlewareFunc for each of them. They are not supposed to run for each request in a concurrent way because the state of each middleware objects would be shared between the go routines.
I think that a more realistic test is to instantiate the API stack of middlewares once, and to run concurrent requests. In that case the test passes.
func TestSimulReads(t *testing.T) {
var api *rest.Api
api, _ = registerRoutes()
h := api.MakeHandler()
f := func() {
recorded := test.RunRequest(t, h,
test.MakeSimpleRequest("GET", "/", nil),
)
recorded.CodeIs(200)
recorded.ContentTypeIsJson()
}
go f()
go f()
}Now, I dug a bit further and found something else. The following code instantiates a full new API object in each go routine (which in practice doesn't make much sense), and fails go test -race.
func TestSimulReads(t *testing.T) {
f := func() {
api := rest.NewApi()
api.Use(rest.DefaultCommonStack...)
router, _ := rest.MakeRouter(
rest.Get("/", GetData),
)
api.SetApp(router)
recorded := test.RunRequest(t, api.MakeHandler(),
test.MakeSimpleRequest("GET", "/", nil),
)
recorded.CodeIs(200)
recorded.ContentTypeIsJson()
}
go f()
go f()
}This is because the stack of middlewares is actually instantiated as a global variable rest.DefaultCommonStack (see https://github.com/ant0ine/go-json-rest/blob/master/rest/api.go#L51), making these middlewares shared between the two API objects and the two go routines.
Replacing the global variable by the actual instantiations of the middlewares works:
func TestSimulReads(t *testing.T) {
f := func() {
api := rest.NewApi()
api.Use(
&rest.TimerMiddleware{},
&rest.RecorderMiddleware{},
&rest.PoweredByMiddleware{},
&rest.RecoverMiddleware{},
)
router, _ := rest.MakeRouter(
rest.Get("/", GetData),
)
api.SetApp(router)
recorded := test.RunRequest(t, api.MakeHandler(),
test.MakeSimpleRequest("GET", "/", nil),
)
recorded.CodeIs(200)
recorded.ContentTypeIsJson()
}
go f()
go f()
}I guess the fix would be to replace these set of global variables: DefaultDevStack, DefaultProdStack, and DefaultCommonStack by a set of functions instantiating a new middleware objects at each call. (eg; MakeDefaultDevStack, ...)
That is obviously a breaking API change.
Any thought ?
I'm able to consistently induce the golang race detector to detect data race in several middleware
In the
DefaultDevStack(likely not expected to be used in production code)and in the
DefaultCommonStack(seems like this should be viable for production)Just based on the naming, it seems like
RecoverMiddlewareis non-optional. If it is optional, is there a middleware stack that you would suggest running in production that is not susceptible to data races?Code is attached (it's basically just the HelloWorld example plus a test). Runnable with
go test -raceThanks!
go_json_rest_race_test.tar.gz