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

Fix data race on mset.cfg #6424

Merged
merged 1 commit into from
Jan 30, 2025
Merged

Fix data race on mset.cfg #6424

merged 1 commit into from
Jan 30, 2025

Conversation

evankanderson
Copy link
Contributor

Using NATS in our unit tests with the Go data race detector turned on, we sometimes see failures in nats-server like the following:

WARNING: DATA RACE
  Read at 0x00c0004ba0e8 by goroutine 41:
    github.com/nats-io/nats-server/v2/server.(*Account).filteredStreams()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/jetstream.go:1504 +0x3e4
    github.com/nats-io/nats-server/v2/server.(*Server).jsStreamNamesRequest()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/jetstream_api.go:1662 +0x516
    github.com/nats-io/nats-server/v2/server.(*Server).jsStreamNamesRequest-fm()
        <autogenerated>:1 +0xcb
    github.com/nats-io/nats-server/v2/server.(*jetStream).apiDispatch()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/jetstream_api.go:818 +0xb37
    github.com/nats-io/nats-server/v2/server.(*jetStream).apiDispatch-fm()
        <autogenerated>:1 +0xcb
    github.com/nats-io/nats-server/v2/server.(*client).deliverMsg()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/client.go:3473 +0xcfe
    github.com/nats-io/nats-server/v2/server.(*client).processMsgResults()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/client.go:4565 +0x1206
    github.com/nats-io/nats-server/v2/server.(*client).processServiceImport()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/client.go:4350 +0x1e0e
    github.com/nats-io/nats-server/v2/server.(*Account).addServiceImportSub.func1()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/accounts.go:2024 +0x4d
    github.com/nats-io/nats-server/v2/server.(*client).deliverMsg()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/client.go:3471 +0xdcf
    github.com/nats-io/nats-server/v2/server.(*client).processMsgResults()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/client.go:4565 +0x1206
    github.com/nats-io/nats-server/v2/server.(*client).processInboundClientMsg()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/client.go:3952 +0x163d
    github.com/nats-io/nats-server/v2/server.(*client).processInboundMsg()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/client.go:3789 +0x88
    github.com/nats-io/nats-server/v2/server.(*client).parse()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/parser.go:497 +0x362d
    github.com/nats-io/nats-server/v2/server.(*client).readLoop()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/client.go:1393 +0x1b18
    github.com/nats-io/nats-server/v2/server.(*Server).createClientEx.func1()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/server.go:3295 +0x54
    github.com/nats-io/nats-server/v2/server.(*Server).startGoRoutine.func1()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/server.go:3795 +0x59
  
  Previous write at 0x00c0004ba0e8 by goroutine 54:
    github.com/nats-io/nats-server/v2/server.(*stream).updateWithAdvisory()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/stream.go:1963 +0x1b96
    github.com/nats-io/nats-server/v2/server.(*stream).update()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/stream.go:1726 +0xf2c
    github.com/nats-io/nats-server/v2/server.(*Server).jsStreamUpdateRequest()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/jetstream_api.go:1543 +0xf2d
    github.com/nats-io/nats-server/v2/server.(*Server).jsStreamUpdateRequest-fm()
        <autogenerated>:1 +0xcb
    github.com/nats-io/nats-server/v2/server.(*jetStream).apiDispatch()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/jetstream_api.go:818 +0xb37
    github.com/nats-io/nats-server/v2/server.(*jetStream).apiDispatch-fm()
        <autogenerated>:1 +0xcb
    github.com/nats-io/nats-server/v2/server.(*client).deliverMsg()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/client.go:3473 +0xcfe
    github.com/nats-io/nats-server/v2/server.(*client).processMsgResults()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/client.go:4565 +0x1206
    github.com/nats-io/nats-server/v2/server.(*client).processServiceImport()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/client.go:4350 +0x1e0e
    github.com/nats-io/nats-server/v2/server.(*Account).addServiceImportSub.func1()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/accounts.go:2024 +0x4d
    github.com/nats-io/nats-server/v2/server.(*client).deliverMsg()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/client.go:3471 +0xdcf
    github.com/nats-io/nats-server/v2/server.(*client).processMsgResults()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/client.go:4565 +0x1206
    github.com/nats-io/nats-server/v2/server.(*client).processInboundClientMsg()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/client.go:3952 +0x163d
    github.com/nats-io/nats-server/v2/server.(*client).processInboundMsg()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/client.go:3789 +0x88
    github.com/nats-io/nats-server/v2/server.(*client).parse()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/parser.go:497 +0x362d
    github.com/nats-io/nats-server/v2/server.(*client).readLoop()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/client.go:1393 +0x1b18
    github.com/nats-io/nats-server/v2/server.(*Server).createClientEx.func1()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/server.go:3295 +0x54
    github.com/nats-io/nats-server/v2/server.(*Server).startGoRoutine.func1()
        /home/runner/go/pkg/mod/github.com/nats-io/nats-server/v2@v2.10.25/server/server.go:3795 +0x59

Signed-off-by: Evan Anderson evan@stacklok.com

@evankanderson evankanderson requested a review from a team as a code owner January 29, 2025 05:08
@evankanderson
Copy link
Contributor Author

Note that we're seeing the errors in v2.10.25, but I couldn't easily figure out which branch to target the PR against, so I targeted it against main. Let me know if there's a different branch I should target.

neilalexander
neilalexander requested changes Jan 29, 2025
View reviewed changes
Copy link
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, just a small nit.

server/jetstream.go Outdated
if SubjectsCollide(filter, subj) {
msets = append(msets, mset)
break
func() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're doing a break rather than an escaping return, we don't really need the anonymous function closure here for defer. We can just put the RUnlock after the for loop instead, which flattens it out a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oop, yes! I got switched up and read the break as in a different context than needs locking, but you're totally correct.

I've squashed the commit, since it doesn't seem that helpful to have one version and then the other.

@evankanderson
Fix data rase on mset.cfg
1606e03 
Signed-off-by: Evan Anderson <evan@stacklok.com>
neilalexander
neilalexander approved these changes Jan 29, 2025
View reviewed changes
Copy link
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@wallyqs wallyqs changed the title Fix data rase on mset.cfg Fix data race on mset.cfg Jan 29, 2025
@derekcollison derekcollison merged commit 1360459 into nats-io:main Jan 30, 2025
2 checks passed
@evankanderson
Copy link
Contributor Author

Will this be merged into the 2.10 branch as well? (Or is the next release 2.11.0?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neilalexander neilalexander neilalexander approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@evankanderson @neilalexander @derekcollison