Break grabLockOrStop into two pieces to facilitate investigating deadlocks

[ncabatoff]

Without this change, the "grab" locking goroutine looks the same regardless of who was calling grabLockOrStop, so there's no way to identify one of the deadlock parties.

Example of the confusion we're trying to eliminate:

=== CONT  TestSecret_TokenAccessor/auth
POTENTIAL DEADLOCK:
Previous place where the lock was grabbed
goroutine 8799 lock 0xc000ccd298
../../init.go:230 vault.(*Core).Initialize { c.stateLock.Lock() } <<<<<
../../testing.go:341 vault.TestCoreInitClusterWrapperSetup { result, err := core.Initialize(context.Background(), initParams) }
../../testing.go:2037 vault.(*TestCluster).initCores { bKeys, rKeys, root := TestCoreInitClusterWrapperSetup(t, leader.Core, leader.Handler) }
../../testing.go:1759 vault.NewTestCluster { testCluster.initCores(t, opts, addAuditBackend) }
api_integration_test.go:61 api.testVaultServerCoreConfig { cluster := vault.NewTestCluster(benchhelpers.TBtoT(t), coreConfig, &vault.TestClusterOptions{ }
api_integration_test.go:36 api.testVaultServerUnseal { return testVaultServerCoreConfig(t, &vault.CoreConfig{ }
api_integration_test.go:27 api.testVaultServer { client, _, closer := testVaultServerUnseal(t) }
secret_test.go:1005 api.TestSecret_TokenPolicies.func3 { client, closer := testVaultServer(t) }

Have been trying to lock it again for more than 30s
goroutine 10172 lock 0xc000ccd298
../../ha.go:670 vault.grabLockOrStop.func1 { lockFunc() } <<<<<

[raskchanky]

It looks like there's a test using the old way of checking grabLockOrStop() here. Does that need to be updated to be called the new way?

[ncabatoff]

It looks like there's a test using the old way of checking grabLockOrStop() here. Does that need to be updated to be called the new way?

No, I kept the old func around for now, in part so I don't break ent when we merge to it. I might get rid of it in another pass later.

[hghaf099]

Looks good to me. I just posted a nit suggestion.

[hghaf099]

Would it worth adding godoc for this function explaining how this function should be used? Although all the examples usages show that, it is not clear maybe for a future developer as to why this function should be used in a separate go routine.

[ncabatoff]

Good idea, added.

[raskchanky]

It looks like there's a test using the old way of checking grabLockOrStop() here. Does that need to be updated to be called the new way?

No, I kept the old func around for now, in part so I don't break ent when we merge to it. I might get rid of it in another pass later.

Right, I noticed that the function itself is still there, but the way the function is used has changed, no? Previously it was:

stopped := grabLockOrStop(c.stateLock.RLock, c.stateLock.RUnlock, stopCh)
if stopped {
    // something
}

And now it's:

l := newLockGrabber(c.stateLock.RLock, c.stateLock.RUnlock, stopCh)
go l.grab()
if stopped := l.lockOrStop(); stopped {
    // something
}

Everything in this PR uses the new way and that test still uses the old way.