Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Potential race condition in Connection module #31

Closed
Zerpet opened this issue Dec 15, 2021 · 2 comments
Closed

Potential race condition in Connection module #31

Zerpet opened this issue Dec 15, 2021 · 2 comments

Comments

@Zerpet
Copy link
Contributor

Zerpet commented Dec 15, 2021

Running our tests with -race flag, it reports a race condition in the Connection module. Pasting here one stack trace to investigate.

WARNING: DATA RACE
Write at 0x00c00022a050 by goroutine 22:
  github.com/rabbitmq/amqp091-go.(*Connection).shutdown.func1()
      /Users/acedres/workspace/amqp091-go/connection.go:443 +0x50e
  sync.(*Once).doSlow()
      /usr/local/Cellar/go/1.17.3/libexec/src/sync/once.go:68 +0x127
  sync.(*Once).Do()
      /usr/local/Cellar/go/1.17.3/libexec/src/sync/once.go:59 +0x46
  github.com/rabbitmq/amqp091-go.(*Connection).shutdown()
      /Users/acedres/workspace/amqp091-go/connection.go:407 +0x7d
  github.com/rabbitmq/amqp091-go.(*Connection).reader()
      /Users/acedres/workspace/amqp091-go/connection.go:542 +0x344
  github.com/rabbitmq/amqp091-go.Open·dwrap·20()
      /Users/acedres/workspace/amqp091-go/connection.go:250 +0x58

Previous write at 0x00c00022a050 by goroutine 20:
  github.com/rabbitmq/amqp091-go.(*Connection).openComplete()
      /Users/acedres/workspace/amqp091-go/connection.go:847 +0x13d
  github.com/rabbitmq/amqp091-go.(*Connection).openVhost()
      /Users/acedres/workspace/amqp091-go/connection.go:832 +0x1ce
=== RUN   TestChannelOpen
  github.com/rabbitmq/amqp091-go.(*Connection).openTune()
      /Users/acedres/workspace/amqp091-go/connection.go:818 +0xb04
  github.com/rabbitmq/amqp091-go.(*Connection).openStart()
      /Users/acedres/workspace/amqp091-go/connection.go:754 +0x4e7
  github.com/rabbitmq/amqp091-go.(*Connection).open()
      /Users/acedres/workspace/amqp091-go/connection.go:726 +0xc4
  github.com/rabbitmq/amqp091-go.Open()
      /Users/acedres/workspace/amqp091-go/connection.go:251 +0x664
  github.com/rabbitmq/amqp091-go.TestOpen()
      /Users/acedres/workspace/amqp091-go/client_test.go:275 +0x267
  testing.tRunner()
      /usr/local/Cellar/go/1.17.3/libexec/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()
      /usr/local/Cellar/go/1.17.3/libexec/src/testing/testing.go:1306 +0x47

Goroutine 22 (running) created at:
  github.com/rabbitmq/amqp091-go.Open()
      /Users/acedres/workspace/amqp091-go/connection.go:250 +0x618
  github.com/rabbitmq/amqp091-go.TestOpen()
      /Users/acedres/workspace/amqp091-go/client_test.go:275 +0x267
  testing.tRunner()
      /usr/local/Cellar/go/1.17.3/libexec/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()
      /usr/local/Cellar/go/1.17.3/libexec/src/testing/testing.go:1306 +0x47

Goroutine 20 (running) created at:
  testing.(*T).Run()
      /usr/local/Cellar/go/1.17.3/libexec/src/testing/testing.go:1306 +0x726
  testing.runTests.func1()
      /usr/local/Cellar/go/1.17.3/libexec/src/testing/testing.go:1598 +0x99
  testing.tRunner()
      /usr/local/Cellar/go/1.17.3/libexec/src/testing/testing.go:1259 +0x22f
  testing.runTests()
      /usr/local/Cellar/go/1.17.3/libexec/src/testing/testing.go:1596 +0x7ca
  testing.(*M).Run()
      /usr/local/Cellar/go/1.17.3/libexec/src/testing/testing.go:1504 +0x9d1
  main.main()
      _testmain.go:251 +0x22b
@DanielePalaia
Copy link
Contributor

This issue is due to the fact that the assignment c.allocator = newAllocator(1, c.Config.ChannelMax) is done at the same time both during OpenConnection in OpenComplete function during shutdown.

While shutdown is protected my the structure mutex "m" OpenComplete is not. A solution would be to protect this function with the same mutex, but to carefully double check if it can create other issues.

DanielePalaia pushed a commit to DanielePalaia/amqp091-go that referenced this issue Mar 25, 2022
Some unit tests in connection were showing a race condition (see rabb…
e09eb3a 
…itmq#31):

These ones were the ones testing Open scenarios. The issue is that Open and Close, rwc.Open and rwc.Close can at the same time write on:

c.allocator = newAllocator(1, c.Config.ChannelMax)
connection.go line 444 and
connection.go line 849

while shutdown is protected by the structure mutex m, OpenComplete() is not causing the race.

Not sure if the library should be protected in this case adding Lock also in OpenComplete().
In any case the tests were just testing Open, so the rwc.Close() can be moved in the main function at the end avoiding the race and not affected the test functionality
@DanielePalaia DanielePalaia mentioned this issue Mar 25, 2022
Closed
DanielePalaia pushed a commit to DanielePalaia/amqp091-go that referenced this issue Mar 25, 2022
Some unit tests in connection were showing a race condition (see rabb…
3b21050 
…itmq#31):

These ones were the ones testing Open scenarios. The issue is that Open and Close, rwc.Open and rwc.Close can at the same time write on:

c.allocator = newAllocator(1, c.Config.ChannelMax)
connection.go line 444 and
connection.go line 849

while shutdown is protected by the structure mutex m, OpenComplete() is not causing the race.

While it's not clear if the library should protect this eventuality, the tests are testing the Open function, so I think the close can be put in the main thread avoiding the race and not affecting the test validity
@Zerpet Zerpet linked a pull request Mar 25, 2022 that will close this issue
Some unit tests in connection were showing a race condition (see #31): #56
Closed
@DanielePalaia
Copy link
Contributor

#56 merged!

@jxsl13 jxsl13 mentioned this issue Feb 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Some unit tests in connection were showing a race condition (see #31): DanielePalaia/amqp091-go
2 participants
@Zerpet @DanielePalaia