Skip to content

Akka.Actor: Context.Watch on FutureActorRef<T> creates memory leaks#7502

Merged
Arkatufus merged 6 commits into
akkadotnet:devfrom
Aaronontheweb:FutureActorRef-deathwatch
Feb 14, 2025
Merged

Akka.Actor: Context.Watch on FutureActorRef<T> creates memory leaks#7502
Arkatufus merged 6 commits into
akkadotnet:devfrom
Aaronontheweb:FutureActorRef-deathwatch

Conversation

@Aaronontheweb

@Aaronontheweb Aaronontheweb commented Feb 14, 2025

Copy link
Copy Markdown
Member

Changes

Fixes #7501

Checklist

For significant changes, please ensure that the following have been completed (delete if not relevant):

Latest dev Benchmarks

BenchmarkDotNet v0.13.12, Pop!_OS 22.04 LTS
13th Gen Intel Core i7-1360P, 1 CPU, 16 logical and 12 physical cores
.NET SDK 9.0.100
  [Host]  : .NET 8.0.11 (8.0.1124.51707), X64 RyuJIT AVX2
  LongRun : .NET 8.0.11 (8.0.1124.51707), X64 RyuJIT AVX2

Job=LongRun  Affinity=1111111111111111  Concurrent=True  
Server=True  InvocationCount=1  IterationCount=50  
LaunchCount=3  RunStrategy=Monitoring  UnrollFactor=1  
WarmupCount=25  
Method ActorCount Mean Error StdDev Median Ratio RatioSD Req/sec
PushMsgs 1 94.40 ns 2.449 ns 8.935 ns 93.86 ns 1.00 0.00 10,593,716.16
AskMsgs 1 355.81 ns 17.848 ns 65.117 ns 340.14 ns 3.80 0.77 2,810,481.06
PushMsgs 10 336.78 ns 78.794 ns 287.466 ns 294.31 ns 1.00 0.00 2,969,279.03
AskMsgs 10 2,715.52 ns 74.493 ns 271.775 ns 2,757.84 ns 9.50 2.72 368,253.95
PushMsgs 100 10,026.03 ns 291.156 ns 1,062.227 ns 10,290.13 ns 1.00 0.00 99,740.39
AskMsgs 100 16,289.24 ns 1,615.623 ns 5,894.292 ns 16,497.87 ns 1.65 0.67 61,390.22

This PR's Benchmarks

BenchmarkDotNet v0.13.12, Pop!_OS 22.04 LTS
13th Gen Intel Core i7-1360P, 1 CPU, 16 logical and 12 physical cores
.NET SDK 9.0.100
  [Host]  : .NET 8.0.11 (8.0.1124.51707), X64 RyuJIT AVX2
  LongRun : .NET 8.0.11 (8.0.1124.51707), X64 RyuJIT AVX2

Job=LongRun  Affinity=1111111111111111  Concurrent=True  
Server=True  InvocationCount=1  IterationCount=50  
LaunchCount=3  RunStrategy=Monitoring  UnrollFactor=1  
WarmupCount=25  
Method ActorCount Mean Error StdDev Median Ratio RatioSD Req/sec
PushMsgs 1 99.15 ns 2.404 ns 8.770 ns 97.76 ns 1.00 0.00 10,085,925.42
AskMsgs 1 341.15 ns 15.517 ns 56.612 ns 328.58 ns 3.47 0.67 2,931,294.70
PushMsgs 10 318.01 ns 60.932 ns 222.297 ns 283.28 ns 1.00 0.00 3,144,519.95
AskMsgs 10 2,632.51 ns 87.963 ns 320.915 ns 2,736.53 ns 9.28 2.52 379,865.83
PushMsgs 100 10,001.31 ns 328.178 ns 1,197.295 ns 10,301.67 ns 1.00 0.00 99,986.89
AskMsgs 100 17,766.91 ns 1,471.117 ns 5,367.088 ns 18,377.23 ns 1.81 0.62 56,284.41

@Aaronontheweb

Copy link
Copy Markdown
Member Author

With 100% certainty, we will need to run the Ask<T> perf benchmarks on this PR before approving it

@Aaronontheweb Aaronontheweb left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed my changes - still need benchmarks

}

[Fact]
public async Task FutureActorRefShouldSupportDeathWatch()

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handles both scenarios:

  1. Context.Watch before the FutureActorRef<T> completes
  2. Context.Watch after the FutureActorRef<T> has already completed, which should immediately report back with a Terminated message

/// <summary>
/// INTERNAL API - didn't want static helper methods declared inside generic class
/// </summary>
internal static class FutureActorRefDeathWatchSupport

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Helper class for sending back ISystemMsgs from FutureActorRef<T>


switch (message)
{
case ISystemMessage msg:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in #7501, this code never worked because it was in the wrong method. Removing it should actually speed up Ask<T> processing slightly.

{
if (_result.Task.IsCompleted)
{
watch.Watcher.SendSystemMessage(FutureActorRefDeathWatchSupport.TerminatedFor(this));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fast path: this actor is already "finished"

}
else
{
_ = FutureActorRefDeathWatchSupport.ScheduleDeathWatch(watch.Watcher, watch.Watchee, _result.Task);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slow path - have to wait for actor to finish

{
if (message is Watch watch)
{
if (_result.Task.IsCompleted)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automatically covers any possible completion state for the task: cancelled, faulted, or ran to completion

}
catch
{
// we don't do error handling for this - we do not care

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error-handling is the job of the user code await-ing on the Ask - not ours.

@Aaronontheweb

Copy link
Copy Markdown
Member Author

No real performance impact, but there are some failing tests that need to be addressed

@Arkatufus Arkatufus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Arkatufus Arkatufus enabled auto-merge (squash) February 14, 2025 17:11
@Arkatufus Arkatufus merged commit 50b6cfe into akkadotnet:dev Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Akka.Actor: FutureActorRef<T> does not support Context.Watch and may cause memory leaks

2 participants