search

nats-io / nats-server

High-Performance server for NATS.io, the cloud and edge native messaging system.
https://nats.io
Apache License 2.0
15.1k stars 1.36k forks source link

Fix inconsistent atomic accesses of client `mpay`, `msubs`, `mcl` #5592

Closed neilalexander closed 4 days ago

neilalexander commented 5 days ago

This fixes data races reported by the race detector. By switching to atomic.Int32 type, we can guarantee that the accesses are atomic across the board, whereas before there were some instances of plain reads/writes mixed with other cases of atomic.StoreInt32/atomic.LoadInt32.

Signed-off-by: Neil Twigg neil@nats.io

derekcollison commented 5 days ago

Did you capture the race output or which text caused it?

neilalexander commented 5 days ago 
==================
WARNING: DATA RACE
Write at 0x00c001996ca8 by goroutine 57967:
  github.com/nats-io/nats-server/v2/server.(*client).applyAccountLimits()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/client.go:850 +0x197
  github.com/nats-io/nats-server/v2/server.(*Server).updateAccountClaimsWithRefresh()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/accounts.go:3598 +0x5824
  github.com/nats-io/nats-server/v2/server.(*Server).updateAccountClaimsWithRefresh.func5()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/accounts.go:3685 +0x1c4
  sync.(*Map).Range()
      /home/travis/.gimme/versions/go1.22.4.linux.amd64/src/sync/map.go:477 +0x1e5
  github.com/nats-io/nats-server/v2/server.(*Server).updateAccountClaimsWithRefresh()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/accounts.go:3671 +0x5e9c
  github.com/nats-io/nats-server/v2/server.(*Server).UpdateAccountClaims()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/accounts.go:3100 +0x15e
  github.com/nats-io/nats-server/v2/server.(*Server).buildInternalAccount()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/accounts.go:3718 +0x145
  github.com/nats-io/nats-server/v2/server.(*Server).fetchAccount()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/server.go:2066 +0x84
  github.com/nats-io/nats-server/v2/server.(*Server).lookupAccount()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/server.go:1942 +0x226
  github.com/nats-io/nats-server/v2/server.(*Server).LookupAccount()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/server.go:1948 +0x23cc
  github.com/nats-io/nats-server/v2/server.(*Server).processClientOrLeafAuthentication()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/auth.go:898 +0x23cd
  github.com/nats-io/nats-server/v2/server.(*Server).isClientAuthorized()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/auth.go:389 +0xb9
  github.com/nats-io/nats-server/v2/server.(*Server).checkAuthentication()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/auth.go:365 +0x7a
  github.com/nats-io/nats-server/v2/server.(*client).processConnect()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/client.go:2057 +0xd50
  github.com/nats-io/nats-server/v2/server.(*client).parse()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/parser.go:926 +0x1437
  github.com/nats-io/nats-server/v2/server.(*client).readLoop()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/client.go:1387 +0x1b18
  github.com/nats-io/nats-server/v2/server.(*Server).createClientEx.func1()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/server.go:3279 +0x54
  github.com/nats-io/nats-server/v2/server.(*Server).startGoRoutine.func1()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/server.go:3764 +0x59

Previous read at 0x00c001996ca8 by goroutine 58071:
  sync/atomic.LoadInt32()
      /home/travis/.gimme/versions/go1.22.4.linux.amd64/src/runtime/race_amd64.s:202 +0xb
  sync/atomic.LoadInt32()
      <autogenerated>:1 +0x10
  github.com/nats-io/nats-server/v2/server.(*client).parse()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/parser.go:427 +0x285a
  github.com/nats-io/nats-server/v2/server.(*client).readLoop()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/client.go:1387 +0x1b18
  github.com/nats-io/nats-server/v2/server.(*Server).createClientEx.func1()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/server.go:3279 +0x54
  github.com/nats-io/nats-server/v2/server.(*Server).startGoRoutine.func1()
      /home/travis/gopath/src/github.com/nats-io/nats-server/server/server.go:3764 +0x59

It looks like all three have inconsistent uses though (treated as atomic in some places and as write-locked in others).

derekcollison commented 5 days ago

What test reported this one?

neilalexander commented 5 days ago

TestJWTImportsOnServerRestartAndClientsReconnect