Fix memory leak caused by QueryThrottler #518
Conversation
| using var cts = CancellationTokenSource.CreateLinkedTokenSource(state.ShutdownToken); | ||
| { | ||
| cts.CancelAfter(state.QueryThrottleTimeout); | ||
| await state.QueryPermitter.Ask<QueryStartGranted>(RequestQueryStart.Instance, cts.Token); | ||
| } |
There was a problem hiding this comment.
Remove complicated cancellation token code, replaced it with Ask internal timeout mechanism
| if(Sender is ActorRefWithCell) | ||
| { | ||
| _watchCount++; | ||
| Context.Watch(Sender); | ||
| } |
There was a problem hiding this comment.
Actual fix, never watch any IActorRef with no ActorCell, these actors are not compatible with death watch.
| _pending.RemoveFirst(); | ||
| QueryStartGranted(popRef); | ||
| if (pending is not null && pending.IsExpired) |
There was a problem hiding this comment.
Since we can't rely on death watch mechanism to automatically purge pending requests, we have to add timeout deadlines to the pending requests. All expired pending requests will be discarded.
| /// Works identically to the RecoveryPermitter built into Akka.Persistence. | ||
| /// Works almost identically to the RecoveryPermitter built into Akka.Persistence. | ||
| /// | ||
| /// NOTE: Since this permitter works with Ask and we can't death watch an Ask temp |
There was a problem hiding this comment.
It would be nice if we could encapsulate/sugar this behavior in future, that said comment is great breadcrumb for future improvement.
|
|
||
| if (_usedPermits < 0) | ||
| throw new IllegalStateException("Permits must not be negative"); | ||
| { | ||
| _log.Warning("Permits must not be negative"); |
There was a problem hiding this comment.
Is it overkill to include the negative value here? Also what justifies the reset to zero?
There was a problem hiding this comment.
Its a warning for the developers. It should not kill the actor outright, and the throttler should keep on going. If the value goes negative, this means that we over-return permits, something is returning more permits than what was issued. Its not a critical error, the actor should work regardless, but we should take a note of it happening.
Fixes #516
Changes
Make sure we don't death watch Ask temp actors
Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):