Fix for data race in memstore.LoadNextMsg

[wallyqs]

Fix for a data race that was found in memory storage while working on a fix for #4551

WARNING: DATA RACE
Read at 0x00c0000a6e38 by goroutine 412:
  github.com/nats-io/nats-server/v2/server.(*memStore).LoadNextMsg()
      /Users/wallyqs/repos/nats-dev/src/github.com/nats-io/nats-server/server/memstore.go:954 +0x644
  github.com/nats-io/nats-server/v2/server.(*consumer).getNextMsg()
      /Users/wallyqs/repos/nats-dev/src/github.com/nats-io/nats-server/server/consumer.go:3413 +0xb22
  github.com/nats-io/nats-server/v2/server.(*consumer).loopAndGatherMsgs()
      /Users/wallyqs/repos/nats-dev/src/github.com/nats-io/nats-server/server/consumer.go:3824 +0x484
  github.com/nats-io/nats-server/v2/server.(*consumer).setLeader.func3()
      /Users/wallyqs/repos/nats-dev/src/github.com/nats-io/nats-server/server/consumer.go:1259 +0x44

Previous write at 0x00c0000a6e38 by goroutine 336:
  github.com/nats-io/nats-server/v2/server.(*memStore).recalculateFirstForSubj()
      /Users/wallyqs/repos/nats-dev/src/github.com/nats-io/nats-server/server/memstore.go:1070 +0x6a5
  github.com/nats-io/nats-server/v2/server.(*memStore).LoadNextMsg()
      /Users/wallyqs/repos/nats-dev/src/github.com/nats-io/nats-server/server/memstore.go:955 +0x653
  github.com/nats-io/nats-server/v2/server.(*consumer).getNextMsg()
      /Users/wallyqs/repos/nats-dev/src/github.com/nats-io/nats-server/server/consumer.go:3413 +0xb22
  github.com/nats-io/nats-server/v2/server.(*consumer).loopAndGatherMsgs()
      /Users/wallyqs/repos/nats-dev/src/github.com/nats-io/nats-server/server/consumer.go:3824 +0x484
  github.com/nats-io/nats-server/v2/server.(*consumer).setLeader.func3()
      /Users/wallyqs/repos/nats-dev/src/github.com/nats-io/nats-server/server/consumer.go:1259 +0x44

[neilalexander]

On the whole I'm a bit nervous about releasing and relocking in this way as it can sometimes lead to subtle bugs or deadlocks, but short of refactoring this completely, this is probably the quickest fix.

[derekcollison]

Is there a way to not do this?

Can we introduce a new func that expects the locks held?

[neilalexander]

I think the function already expects the write lock held, which is the issue. At that point we have a read lock and not a write lock.

[derekcollison]

I think I would prefer, for now, to move the lock for LoadNextMsg to a write lock and then do a lock already held version of recalulateFirstForSubj().

[derekcollison]

LGTM