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.

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

Protect map and queue variable access with mutex, add test #88

Merged
merged 1 commit into from
Oct 8, 2023

Conversation

C-Pro
Copy link
Contributor

@C-Pro C-Pro commented Sep 15, 2023

This PR should fix a couple of possible races by adding mutex lock where variable access was not protected.

Background:

I've got a race in centrifuge-go while running my application tests:

WARNING: DATA RACE
Write at 0x00c000488630 by goroutine 9:
  runtime.mapassign_faststr()
      /usr/local/go/src/runtime/map_faststr.go:203 +0x0
  github.com/centrifugal/centrifuge-go.(*Client).NewSubscription()
      /home/runner/go/pkg/mod/github.com/centrifugal/centrifuge-go@v0.9.6/client.go:191 +0x6bd
  github.com/pintu-crypto/exchange/backend/account/view/fake.TestFakeCentrifuge()
      /runner/_work/exchange/exchange/backend/account/view/fake/fake_test.go:176 +0x135e
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1576 +0x216
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1629 +0x47

Previous read at 0x00c000488630 by goroutine 122:
  runtime.mapaccess2_faststr()
      /usr/local/go/src/runtime/map_faststr.go:108 +0x0
  github.com/centrifugal/centrifuge-go.(*Client).handlePush()
      /home/runner/go/pkg/mod/github.com/centrifugal/centrifuge-go@v0.9.6/client.go:699 +0x265
  github.com/centrifugal/centrifuge-go.(*Client).handle()
      /home/runner/go/pkg/mod/github.com/centrifugal/centrifuge-go@v0.9.6/client.go:667 +0x467
  github.com/centrifugal/centrifuge-go.(*Client).readOnce()
      /home/runner/go/pkg/mod/github.com/centrifugal/centrifuge-go@v0.9.6/client.go:605 +0x12a
  github.com/centrifugal/centrifuge-go.(*Client).reader()
      /home/runner/go/pkg/mod/github.com/centrifugal/centrifuge-go@v0.9.6/client.go:612 +0xad
  github.com/centrifugal/centrifuge-go.(*Client).startReconnecting.func5()
      /home/runner/go/pkg/mod/github.com/centrifugal/centrifuge-go@v0.9.6/client.go:950 +0x64

I believe the reason was an unprotected c.subs map read in handlePush. I added the test to reproduce and another race popped out in runHandlerSync, looks like c.cbQueue also can be mutated concurrently, so I added mutex there too.

Please take a look.

@FZambia
Copy link
Member

FZambia commented Oct 8, 2023

Uff, sorry for the delay, I've seen this PR but totally forgot about it. Many thanks @C-Pro

@FZambia FZambia merged commit 092d30b into centrifugal:master Oct 8, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants