ibm-messaging / mq-golang

Calling IBM MQ from Go applications
Apache License 2.0
167 stars 60 forks source link

Fix a data race in getAttrInfo #204

Closed kalmant closed 10 months ago

kalmant commented 10 months ago

Please ensure all items are complete before opening.

What

Fixes an issue raised in https://github.com/ibm-messaging/mq-golang/issues/203

How

By using sync.Once instead of a bool to ensure the initialization block gets called only once.

Testing

Running go test -race ./... after I added TestGetAttrInfoConcurrentCalls without the fix would yield output such as:

==================
WARNING: DATA RACE
Read at 0x0001058dcc8d by goroutine 21:
  github.com/ibm-messaging/mq-golang/v5/ibmmq.getAttrInfo()
      /Users/foo/bar/mq-golang/ibmmq/mqiattrs.go:144 +0x34
  github.com/ibm-messaging/mq-golang/v5/ibmmq.TestGetAttrInfoConcurrentCalls.func1()
      /Users/foo/bar/mq-golang/ibmmq/ibmmq_test.go:335 +0x4c

Previous write at 0x0001058dcc8d by goroutine 15:
  github.com/ibm-messaging/mq-golang/v5/ibmmq.getAttrInfo()
      /Users/foo/bar/mq-golang/ibmmq/mqiattrs.go:158 +0x1b8
  github.com/ibm-messaging/mq-golang/v5/ibmmq.TestGetAttrInfoConcurrentCalls.func1()
      /Users/foo/bar/mq-golang/ibmmq/ibmmq_test.go:335 +0x4c

Goroutine 21 (running) created at:
  github.com/ibm-messaging/mq-golang/v5/ibmmq.TestGetAttrInfoConcurrentCalls()
      /Users/foo/bar/mq-golang/ibmmq/ibmmq_test.go:334 +0x74
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.21.3/libexec/src/testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      /opt/homebrew/Cellar/go/1.21.3/libexec/src/testing/testing.go:1648 +0x40

Goroutine 15 (finished) created at:
  github.com/ibm-messaging/mq-golang/v5/ibmmq.TestGetAttrInfoConcurrentCalls()
      /Users/foo/bar/mq-golang/ibmmq/ibmmq_test.go:334 +0x74
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.21.3/libexec/src/testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      /opt/homebrew/Cellar/go/1.21.3/libexec/src/testing/testing.go:1648 +0x40
==================
==================
WARNING: DATA RACE
Read at 0x00c00009e360 by goroutine 18:
  runtime.mapaccess1_fast32()
      /opt/homebrew/Cellar/go/1.21.3/libexec/src/runtime/map_fast32.go:13 +0x1bc
  github.com/ibm-messaging/mq-golang/v5/ibmmq.getAttrInfo()
      /Users/foo/bar/mq-golang/ibmmq/mqiattrs.go:162 +0x254
  github.com/ibm-messaging/mq-golang/v5/ibmmq.TestGetAttrInfoConcurrentCalls.func1()
      /Users/foo/bar/mq-golang/ibmmq/ibmmq_test.go:335 +0x4c

Previous write at 0x00c00009e360 by goroutine 15:
  runtime.mapaccess2_fast32()
      /opt/homebrew/Cellar/go/1.21.3/libexec/src/runtime/map_fast32.go:53 +0x1cc
  github.com/ibm-messaging/mq-golang/v5/ibmmq.getAttrInfo()
      /Users/foo/bar/mq-golang/ibmmq/mqiattrs.go:156 +0x164
  github.com/ibm-messaging/mq-golang/v5/ibmmq.TestGetAttrInfoConcurrentCalls.func1()
      /Users/foo/bar/mq-golang/ibmmq/ibmmq_test.go:335 +0x4c

Goroutine 18 (running) created at:
  github.com/ibm-messaging/mq-golang/v5/ibmmq.TestGetAttrInfoConcurrentCalls()
      /Users/foo/bar/mq-golang/ibmmq/ibmmq_test.go:334 +0x74
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.21.3/libexec/src/testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      /opt/homebrew/Cellar/go/1.21.3/libexec/src/testing/testing.go:1648 +0x40

Goroutine 15 (finished) created at:
  github.com/ibm-messaging/mq-golang/v5/ibmmq.TestGetAttrInfoConcurrentCalls()
      /Users/foo/bar/mq-golang/ibmmq/ibmmq_test.go:334 +0x74
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.21.3/libexec/src/testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      /opt/homebrew/Cellar/go/1.21.3/libexec/src/testing/testing.go:1648 +0x40
==================
--- FAIL: TestGetAttrInfoConcurrentCalls (0.01s)
    testing.go:1465: race detected during execution of test

Now it's ok github.com/ibm-messaging/mq-golang/v5/ibmmq 1.453s.

The test does not check whether getAttrInfo's overall behavior stayed the same, it only checks for data races!

Issues

https://github.com/ibm-messaging/mq-golang/issues/203

raja commented 10 months ago

@ibmmqmet How are pull requests reviewed/assigned?

ibmmqmet commented 10 months ago

Thanks for finding that. It's now been merged and pushed to a new version

kalmant commented 10 months ago

Thank you!