Race condition and unnecessary heap alloc in actor locking

[youngbupark]

In what area(s)?

/area runtime

What version of Dapr?

master

Problem

When multiple invocation requests comes, we use lock to meet turn-based concurrency per actor and set the busy flag and create busy channel. these busy flag and busy channel are used to check the busy state of actor and signal to drain actors.

Drain actor logic

dapr/pkg/actors/actors.go

Lines 656 to 667 in b8e8eec

	if a.config.DrainRebalancedActors {
	// wait until actor isn't busy or timeout hits
	if actor.isBusy() {
	select {
	case <-time.After(a.config.DrainOngoingCallTimeout):
	break
	case <-actor.channel():
	// if a call comes in from the actor for state changes, that's still allowed
	break
	}
	}
	}

actor lock / unlock

dapr/pkg/actors/actor.go

Lines 47 to 66 in b8e8eec

	func (a *actor) lock() {
	atomic.AddInt32(&a.pendingLockCount, 1)
	diag.DefaultMonitoring.ReportCurrentPendingLocks(a.actorType, a.pendingLockCount)
	a.concurrencyLock.Lock()
	a.busy = true
	a.busyCh = make(chan bool, 1)
	a.lastUsedTime = time.Now().UTC()
	}
	func (a *actor) unLock() {
	if a.busy {
	a.busy = false
	close(a.busyCh)
	}
	a.concurrencyLock.Unlock()
	atomic.AddInt32(&a.pendingLockCount, -1)
	diag.DefaultMonitoring.ReportCurrentPendingLocks(a.actorType, a.pendingLockCount)
	}

Let's assume that we have two requests, A and B, which invoke ActorID0 simultaneously. A holds lock and then B holds the same lock.
Once A releases the lock, A will mark busy flag to false and close the busy channel. B does not get lock yet

If ActorID0 is being drained at this time, draining logic can deactivate ActorID0 because busy channel is closed. but B will try to set the busy flag to true and set new channel. B can call application deactivation http url. So in this case, runtime actor table doesn't include ActorID0 because it is deleted, but B can call application method and SDK will try to activate it again.

In this case, ActorID0 status between runtime and application is not consistent.

Release Note

RELEASE NOTE: FIX race condition of busy flag and channel and unnecessary heap alloc in actor locking

[youngbupark]

@yaron2 @artursouza Please review this issue. If you think I misunderstand, please point it out. Otherwise, we should fix this in this milestone. I found this problem when I profile memory of dapr runtime. it busy channel creation allocates lots of memory. we need to improve this part as well.