Use correct mutex to guard confirms.published

[hjr265]

The confirms type uses the publishedMut to guard access to the published field:

amqp091-go/confirms.go

Lines 43 to 47 in a6fa7f7

	c.publishedMut.Lock()
	defer c.publishedMut.Unlock()
	c.published++
	return c.deferredConfirmations.Add(c.published)

amqp091-go/confirms.go

Lines 53 to 56 in a6fa7f7

	c.publishedMut.Lock()
	defer c.publishedMut.Unlock()
	c.deferredConfirmations.remove(c.published)
	c.published--

etc.

However, the Channel type uses the mutex m (on the confirms type) instead:

amqp091-go/channel.go

Lines 1829 to 1832 in a6fa7f7

	ch.confirms.m.Lock()
	defer ch.confirms.m.Unlock()
	return ch.confirms.published + 1

This can lead to race if the GetNextPublishSeqNo function is used while other functions on the confirms type is used concurrently.

[hjr265]

I have added a test which, if run without the fix, results in data race warning:

» make tests GO_TEST_FLAGS='-run ^TestIntegrationGetNextPublishSeqNoRace$$ -count 1'
go test -race -v -tags integration -run ^TestIntegrationGetNextPublishSeqNoRace$ -count 1
environment variable envAMQPURLName undefined or empty, using default: "amqp://guest:[email protected]:5672/"
=== RUN   TestIntegrationGetNextPublishSeqNoRace
==================
WARNING: DATA RACE
Write at 0x00c000096260 by goroutine 12:
  github.com/rabbitmq/amqp091-go.(*confirms).publish()
      /home/hjr265/Projects/contrib/amqp091-go/confirms.go:46 +0xea
  github.com/rabbitmq/amqp091-go.(*Channel).PublishWithDeferredConfirmWithContext()
      /home/hjr265/Projects/contrib/amqp091-go/channel.go:1564 +0x18a
  github.com/rabbitmq/amqp091-go.(*Channel).PublishWithContext()
      /home/hjr265/Projects/contrib/amqp091-go/channel.go:1528 +0x106
  github.com/rabbitmq/amqp091-go.TestIntegrationGetNextPublishSeqNoRace.func2()
      /home/hjr265/Projects/contrib/amqp091-go/integration_test.go:2065 +0x107

Previous read at 0x00c000096260 by goroutine 11:
  github.com/rabbitmq/amqp091-go.(*Channel).GetNextPublishSeqNo()
      /home/hjr265/Projects/contrib/amqp091-go/channel.go:1832 +0xfc
  github.com/rabbitmq/amqp091-go.TestIntegrationGetNextPublishSeqNoRace.func1()
      /home/hjr265/Projects/contrib/amqp091-go/integration_test.go:2057 +0xb9

Goroutine 12 (running) created at:
  github.com/rabbitmq/amqp091-go.TestIntegrationGetNextPublishSeqNoRace()
      /home/hjr265/Projects/contrib/amqp091-go/integration_test.go:2063 +0x5f5
  testing.tRunner()
      /usr/lib/go/src/testing/testing.go:1595 +0x261
  testing.(*T).Run.func1()
      /usr/lib/go/src/testing/testing.go:1648 +0x44

Goroutine 11 (finished) created at:
  github.com/rabbitmq/amqp091-go.TestIntegrationGetNextPublishSeqNoRace()
      /home/hjr265/Projects/contrib/amqp091-go/integration_test.go:2055 +0x4e4
  testing.tRunner()
      /usr/lib/go/src/testing/testing.go:1595 +0x261
  testing.(*T).Run.func1()
      /usr/lib/go/src/testing/testing.go:1648 +0x44
==================
    testing.go:1465: race detected during execution of test
--- FAIL: TestIntegrationGetNextPublishSeqNoRace (0.01s)
=== NAME  
    testing.go:1465: race detected during execution of test
FAIL
exit status 1
FAIL    github.com/rabbitmq/amqp091-go  0.023s
make: *** [Makefile:20: tests] Error 1

With the fix, there is no data race.

» make tests GO_TEST_FLAGS='-run ^TestIntegrationGetNextPublishSeqNoRace$$ -count 1'
go test -race -v -tags integration -run ^TestIntegrationGetNextPublishSeqNoRace$ -count 1
environment variable envAMQPURLName undefined or empty, using default: "amqp://guest:[email protected]:5672/"
=== RUN   TestIntegrationGetNextPublishSeqNoRace
--- PASS: TestIntegrationGetNextPublishSeqNoRace (0.01s)
PASS
ok      github.com/rabbitmq/amqp091-go  1.022s

[lukebakken]

Thank you very much! If you have time to review the use of these and other mutexes for similar errors, we would appreciate it. Obviously, this is a subtle bug 😸

[hjr265]

@lukebakken It was a subtle bug indeed. I came across it while investing an issue I am facing on production. The only clues I had were that the production bug started happening when I implemented Publisher Confirms and the code was publishing a lot of messages in bursts.

I will keep an eye out for more. 🙂

[Zerpet]

Thank you for fixing this! I've approved the CI run and I'll merge after CI passes 👍