qhenkart/gosqs

Race Condition reported in unit tests

Opened this issue · 1 comments

I modified the Makefile so it would enable race condition checking when testing.

test:
	@go test ./... --race --count=3

( I also added --count just to work it a bit harder)

There is a race condition being reported.
It's when you renew the visibility of the consumer on line 184 of consumer_test.go

Maybe Consumer.VisibilityTimeout should be private so it can't be modified once the goroutines are running.
This should probably also apply to the other public fields of Consumer.

$ make test
==================
WARNING: DATA RACE
Write at 0x00c0000ee8b0 by goroutine 42:
  github.com/qhenkart/gosqs.TestRun.func5()
      /home/mark/src/gosqs/consumer_test.go:184 +0x54
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()
      /usr/local/go/src/testing/testing.go:1306 +0x47

Previous read at 0x00c0000ee8b0 by goroutine 34:
  github.com/qhenkart/gosqs.(*consumer).extend()
      /home/mark/src/gosqs/consumer.go:270 +0x77
  github.com/qhenkart/gosqs.(*consumer).run·dwrap·2()
      /home/mark/src/gosqs/consumer.go:189 +0x64

Goroutine 42 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:1306 +0x726
  github.com/qhenkart/gosqs.TestRun()
      /home/mark/src/gosqs/consumer_test.go:183 +0x3ca
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()
      /usr/local/go/src/testing/testing.go:1306 +0x47

Goroutine 34 (running) created at:
  github.com/qhenkart/gosqs.(*consumer).run()
      /home/mark/src/gosqs/consumer.go:189 +0x264
  github.com/qhenkart/gosqs.TestRun.func2()
      /home/mark/src/gosqs/consumer_test.go:162 +0xf2
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()
      /usr/local/go/src/testing/testing.go:1306 +0x47
==================
--- FAIL: TestRun (2.45s)
    --- FAIL: TestRun/renew_visibility (2.12s)
        testing.go:1152: race detected during execution of test
    testing.go:1152: race detected during execution of test
FAIL
FAIL	github.com/qhenkart/gosqs	9.703s
?   	github.com/qhenkart/gosqs/examples	[no test files]
ok  	github.com/qhenkart/gosqs/sqstesting	0.040s
FAIL
make: *** [Makefile:5: test] Error 1

@MarkSonghurst Thanks for the issue! I think this stems for from my habit of writing cascading tests that aren't really meant to run concurrently. That being said, you've made a very good point. By allowing clients to modify these fields, I am opening the door for them to create race conditions. As these fields are meant to set up once during configuration, I agree that the fields should be private to enforce only one safe pathway to modify these fields before goroutines are initialized.

Thank you very much for the suggestion. If you would like to make a pull request, I would happily accept it