xShaneSong / gorest

Automatically exported from code.google.com/p/gorest
0 stars 0 forks source link

Library not thread safe. #15

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
What steps will reproduce the problem?
1. set environment variable GOMAXPROCS > 1
2. Take basic hello world example, call ResponseBuilder().AddHeader(...) in 
handling function
3. Make many simultaneous requests, 
4. some of the responses will get twice the number of headers set, some will 
get none.

What version of the product are you using? On what operating system?

go1.1

Please provide any additional information below.

I believe this is causes by setting the 'Context' in 'reflect.go'. Seems like 
it's not correctly creating one instantiated service object per one request. 
Putting a sleep statement after 
'servVal.FieldByName("RestService").FieldByName("Context").Set(reflect.ValueOf(c
ontext))' will clearly show all response headers being sent to the last caller 
within the sleep period.

Original issue reported on code.google.com by co...@soundcloud.com on 16 Oct 2013 at 7:47

GoogleCodeExporter commented 8 years ago
Yep, the reflection was not creating new instances. Introduced in refactor.
Added test cases

Original comment by siyabong...@gmail.com on 22 Oct 2013 at 4:58

GoogleCodeExporter commented 8 years ago
Awesome work, thanks siyabong.

Original comment by co...@soundcloud.com on 22 Oct 2013 at 5:02

GoogleCodeExporter commented 8 years ago
TestStress crashes on my machine.

$ go test -v -test.run=TestStress >stdout 2>stderr ; echo $? 1


It panics here:
1 panic: runtime error: invalid memory address or nil pointer dereference
2 [signal 0xb code=0x1 addr=0x10 pc=0x510c2]
3 
4 goroutine 36 [running]:
5 runtime.panic(0x2a3da0, 0x5f6339)
6     /tmp/src/code.google.com/p/go/src/pkg/runtime/panic.c:266 +0xb6
7 tmp/gorest.func·003(0xc210039240, 0x20)
8     /tmp/src/tmp/gorest/api-stress_test.go:46 +0xc2
9 created by tmp/gorest.loop1

10 /tmp/src/tmp/gorest/api-stress_test.go:54 +0xbe


I was able to stop the panics by checking the error returned by Delete, here:

- https://code.google.com/p/gorest/source/browse/api-stress_test.go#45

But now the TestStress doesn't return after several minutes. I didn't dig 
further.

The library appears to have data races. Every unsynchronized access to a 
package-global variable is unsafe:

- client.go sharedClient: 
https://code.google.com/p/gorest/source/browse/client.go#38
- gorest.go restManager and handlerInitialized: 
https://code.google.com/p/gorest/source/browse/gorest.go#91

If your test coverage is sufficient, you may be able to confirm some of those 
by running `go test -race -i && go test -race`. Otherwise, you can build the 
library and a binary driver both with `go build -race` and run it in a regular 
environment. Race conditions will be printed to stderr as they're encountered.

Original comment by p...@soundcloud.com on 23 Oct 2013 at 8:33

GoogleCodeExporter commented 8 years ago
With a quick check (without time to look it too much detail why) this change 
breaks existing servers. Have removed gorest usage for now. Is it worth 
reopening this issue rather than leave it 'fixed'?

Original comment by co...@soundcloud.com on 24 Oct 2013 at 10:03