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

kv/concurrency: remove TODO about impossible deadlock scenario #47616

Merged
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
kv/concurrency: clean up waitKind and waitingState types
Strictly renames and added comments. Just a refactor.
  • Loading branch information
nvanbenschoten committed Apr 22, 2020
commit f60948d6b7c63fc384ba84de713e3b21c70ed0f1
78 changes: 52 additions & 26 deletions pkg/kv/kvserver/concurrency/lock_table.go
Original file line number Diff line number Diff line change
@@ -32,33 +32,59 @@ import (
// Default upper bound on the number of locks in a lockTable.
const defaultLockTableSize = 10000

// The kind of waiting that the request is subject to. See the detailed comment
// above for the meaning of each kind.
type stateKind int
// The kind of waiting that the request is subject to.
type waitKind int

const (
waitForDistinguished stateKind = iota
_ waitKind = iota

// waitFor indicates that the request is waiting on another transaction to
// to release its locks or complete its own request. waitingStates with this
// waitKind will provide information on who the request is waiting on. The
// request will likely want to eventually push the conflicting transaction.
waitFor

// waitForDistinguished is a sub-case of waitFor. It implies everything that
// waitFor does and additionally indicates that the request is currently the
// "distinguished waiter". A distinguished waiter is responsible for taking
// extra actions, e.g. immediately pushing the transaction it is waiting
// for. If there are multiple requests in the waitFor state waiting on the
// same transaction, at least one will be a distinguished waiter.
waitForDistinguished

// waitElsewhere is used when the lockTable is under memory pressure and is
// clearing its internal queue state. Like the waitFor* states, it informs
// the request who it is waiting for so that deadlock detection works.
// However, sequencing information inside the lockTable is mostly discarded.
waitElsewhere

// waitSelf indicates that a different requests from the same transaction
// has a conflicting reservation. See the comment about "Reservations" in
// lockState. This request should sit tight and wait for a new notification
// without pushing anyone.
waitSelf

// doneWaiting indicates that the request is done waiting on this pass
// through the lockTable and should make another call to ScanAndEnqueue.
doneWaiting
)

// The current waiting state of the request. See the detailed comment above.
// The current waiting state of the request.
//
// See the detailed comment about "Waiting logic" on lockTableGuardImpl.
type waitingState struct {
stateKind stateKind

// Populated for waitFor* and waitElsewhere type, and represents who the
// request is waiting for.
txn *enginepb.TxnMeta // always non-nil
key roachpb.Key // the key of the lock that is causing the wait
held bool // is the lock currently held?
kind waitKind
guardAccess spanset.SpanAccess // the access method of the guard

// Populated for waitFor* and waitElsewhere kinds. Represents who
// the request is waiting for.
txn *enginepb.TxnMeta // always non-nil
key roachpb.Key // the key of the lock that is causing the wait
held bool // is the lock currently held?
}

// Implementation
// TODO(sbhola):
// - proper error strings and give better explanation to all panics.
// - metrics about lockTable state to export to observability debug pages:
// number of locks, number of waiting requests, wait time?, ...
// - test cases where guard.readTS != guard.writeTS.
@@ -436,7 +462,7 @@ func (g *lockTableGuardImpl) findNextLockAfter(notify bool) {
}
g.mu.Lock()
defer g.mu.Unlock()
g.mu.state.stateKind = doneWaiting
g.mu.state.kind = doneWaiting
if notify {
g.notify()
}
@@ -807,12 +833,12 @@ func (l *lockState) informActiveWaiters() {
}
}
waitForState := waitingState{
stateKind: waitFor,
txn: waitForTxn,
key: l.key,
held: l.holder.locked,
kind: waitFor,
txn: waitForTxn,
key: l.key,
held: l.holder.locked,
}
waitSelfState := waitingState{stateKind: waitSelf}
waitSelfState := waitingState{kind: waitSelf}

for e := l.waitingReaders.Front(); e != nil; e = e.Next() {
state := waitForState
@@ -828,7 +854,7 @@ func (l *lockState) informActiveWaiters() {
g.mu.Lock()
g.mu.state = state
if l.distinguishedWaiter == g {
g.mu.state.stateKind = waitForDistinguished
g.mu.state.kind = waitForDistinguished
}
g.notify()
g.mu.Unlock()
@@ -853,7 +879,7 @@ func (l *lockState) informActiveWaiters() {
g.mu.Lock()
g.mu.state = state
if l.distinguishedWaiter == g {
g.mu.state.stateKind = waitForDistinguished
g.mu.state.kind = waitForDistinguished
}
g.notify()
g.mu.Unlock()
@@ -905,7 +931,7 @@ func (l *lockState) tryMakeNewDistinguished() {
if g != nil {
l.distinguishedWaiter = g
g.mu.Lock()
g.mu.state.stateKind = waitForDistinguished
g.mu.state.kind = waitForDistinguished
// The rest of g.state is already up-to-date.
g.notify()
g.mu.Unlock()
@@ -1094,15 +1120,15 @@ func (l *lockState) tryActiveWait(g *lockTableGuardImpl, sa spanset.SpanAccess,
g.key = l.key
g.mu.startWait = true
if reservedBySelfTxn {
g.mu.state = waitingState{stateKind: waitSelf}
g.mu.state = waitingState{kind: waitSelf}
} else {
stateType := waitFor
if l.distinguishedWaiter == nil {
l.distinguishedWaiter = g
stateType = waitForDistinguished
}
g.mu.state = waitingState{
stateKind: stateType,
kind: stateType,
txn: waitForTxn,
key: l.key,
held: l.holder.locked,
@@ -1356,15 +1382,15 @@ func (l *lockState) tryClearLock(force bool) bool {
// Note that none of the current waiters can be requests
// from holderTxn.
waitState = waitingState{
stateKind: waitElsewhere,
kind: waitElsewhere,
txn: holderTxn,
key: l.key,
held: true,
guardAccess: spanset.SpanReadOnly,
}
} else {
l.holder.locked = false
waitState = waitingState{stateKind: doneWaiting}
waitState = waitingState{kind: doneWaiting}
}

l.distinguishedWaiter = nil
8 changes: 4 additions & 4 deletions pkg/kv/kvserver/concurrency/lock_table_test.go
Original file line number Diff line number Diff line change
@@ -375,7 +375,7 @@ func TestLockTableBasic(t *testing.T) {
}
state := g.CurState()
var typeStr string
switch state.stateKind {
switch state.kind {
case waitForDistinguished:
typeStr = "waitForDistinguished"
case waitFor:
@@ -534,7 +534,7 @@ func doWork(ctx context.Context, item *workItem, e *workloadExecutor) error {
return ctx.Err()
}
state := g.CurState()
switch state.stateKind {
switch state.kind {
case doneWaiting:
if !lastID.Equal(uuid.UUID{}) && item.request.Txn != nil {
_, err = e.waitingFor(item.request.Txn.ID, lastID, uuid.UUID{})
@@ -562,7 +562,7 @@ func doWork(ctx context.Context, item *workItem, e *workloadExecutor) error {
}
}
default:
return errors.Errorf("unexpected state: %v", state.stateKind)
return errors.Errorf("unexpected state: %v", state.kind)
}
}
}
@@ -1075,7 +1075,7 @@ func doBenchWork(item *benchWorkItem, env benchEnv, doneCh chan<- error) {
for {
<-g.NewStateChan()
state := g.CurState()
if state.stateKind == doneWaiting {
if state.kind == doneWaiting {
break
}
}
6 changes: 3 additions & 3 deletions pkg/kv/kvserver/concurrency/lock_table_waiter.go
Original file line number Diff line number Diff line change
@@ -120,7 +120,7 @@ func (w *lockTableWaiterImpl) WaitOn(
case <-newStateC:
timerC = nil
state := guard.CurState()
switch state.stateKind {
switch state.kind {
case waitFor, waitForDistinguished:
// waitFor indicates that the request is waiting on another
// transaction. This transaction may be the lock holder of a
@@ -141,7 +141,7 @@ func (w *lockTableWaiterImpl) WaitOn(
// had a cache of aborted transaction IDs that allowed us to notice
// and quickly resolve abandoned intents then we might be able to
// get rid of this state.
livenessPush := state.stateKind == waitForDistinguished
livenessPush := state.kind == waitForDistinguished
deadlockPush := true

// If the conflict is a reservation holder and not a held lock then
@@ -307,7 +307,7 @@ func (w *lockTableWaiterImpl) WaitOnLock(
return roachpb.NewError(err)
}
return w.pushLockTxn(ctx, req, waitingState{
stateKind: waitFor,
kind: waitFor,
txn: &intent.Txn,
key: intent.Key,
held: true,
20 changes: 10 additions & 10 deletions pkg/kv/kvserver/concurrency/lock_table_waiter_test.go
Original file line number Diff line number Diff line change
@@ -117,7 +117,7 @@ func TestLockTableWaiterWithTxn(t *testing.T) {
w, _, g := setupLockTableWaiterTest()
defer w.stopper.Stop(ctx)

g.state = waitingState{stateKind: doneWaiting}
g.state = waitingState{kind: doneWaiting}
g.notify()

err := w.WaitOn(ctx, makeReq(), g)
@@ -187,7 +187,7 @@ func TestLockTableWaiterWithNonTxn(t *testing.T) {
w, _, g := setupLockTableWaiterTest()
defer w.stopper.Stop(ctx)

g.state = waitingState{stateKind: doneWaiting}
g.state = waitingState{kind: doneWaiting}
g.notify()

err := w.WaitOn(ctx, makeReq(), g)
@@ -221,7 +221,7 @@ func TestLockTableWaiterWithNonTxn(t *testing.T) {
})
}

func testWaitPush(t *testing.T, k stateKind, makeReq func() Request, expPushTS hlc.Timestamp) {
func testWaitPush(t *testing.T, k waitKind, makeReq func() Request, expPushTS hlc.Timestamp) {
ctx := context.Background()
keyA := roachpb.Key("keyA")
testutils.RunTrueAndFalse(t, "lockHeld", func(t *testing.T, lockHeld bool) {
@@ -232,7 +232,7 @@ func testWaitPush(t *testing.T, k stateKind, makeReq func() Request, expPushTS h

req := makeReq()
g.state = waitingState{
stateKind: k,
kind: k,
txn: &pusheeTxn.TxnMeta,
key: keyA,
held: lockHeld,
@@ -285,12 +285,12 @@ func testWaitPush(t *testing.T, k stateKind, makeReq func() Request, expPushTS h
require.Equal(t, keyA, intent.Key)
require.Equal(t, pusheeTxn.ID, intent.Txn.ID)
require.Equal(t, roachpb.ABORTED, intent.Status)
g.state = waitingState{stateKind: doneWaiting}
g.state = waitingState{kind: doneWaiting}
g.notify()
return nil
}
} else {
g.state = waitingState{stateKind: doneWaiting}
g.state = waitingState{kind: doneWaiting}
g.notify()
}
return resp, nil
@@ -302,12 +302,12 @@ func testWaitPush(t *testing.T, k stateKind, makeReq func() Request, expPushTS h
})
}

func testWaitNoopUntilDone(t *testing.T, k stateKind, makeReq func() Request) {
func testWaitNoopUntilDone(t *testing.T, k waitKind, makeReq func() Request) {
ctx := context.Background()
w, _, g := setupLockTableWaiterTest()
defer w.stopper.Stop(ctx)

g.state = waitingState{stateKind: k}
g.state = waitingState{kind: k}
g.notify()
defer notifyUntilDone(t, g)()

@@ -324,7 +324,7 @@ func notifyUntilDone(t *testing.T, g *mockLockTableGuard) func() {
<-g.stateObserved
g.notify()
<-g.stateObserved
g.state = waitingState{stateKind: doneWaiting}
g.state = waitingState{kind: doneWaiting}
g.notify()
<-g.stateObserved
close(done)
@@ -357,7 +357,7 @@ func TestLockTableWaiterIntentResolverError(t *testing.T) {
pusheeTxn := makeTxnProto("pushee")
lockHeld := sync
g.state = waitingState{
stateKind: waitForDistinguished,
kind: waitForDistinguished,
txn: &pusheeTxn.TxnMeta,
key: keyA,
held: lockHeld,