-
Notifications
You must be signed in to change notification settings - Fork 41
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
MessageHub is holding objects in Memory #7
Comments
Okay, I understand your issue now. When calling Okay I will fix this. Thank you for reporting it. |
Thanks Nima, I am putting together an example if you still need it. |
Yes that will be great as well. Thanks. |
Here's a Console application that shows the issue:
|
Excellent, will fix it soon. Thanks. |
Fixed the issue, please update the package and confirm. |
Hi Nima, Works perfectly now! Thanks for the quick response, and thank you for your work on MessageHub. :) |
Thank you for reporting it :-) |
This issue still occurs if publish is done on a different thread from the subscribe/unsubscribe. It is especially problematic if the publish is done in a Task that uses the thread pool since every time the publish is done it will be on a different thread and the subscription will be stored in the _localSubscriptions for that thread and never cleared leading to memory leaks. |
Have you got a repro for me to look at? |
Sure. Pretty much the same as the above example except you execute the publish using Task.Factory.
|
The code above is not an accurate method of identifying leaks. It is measuring the amount of memory GC has allocated regardless of whether the GC was executed or not. A more accurate method would be: private static void PrintMemory(string step)
{
GC.Collect();
GC.WaitForPendingFinalizers();
Console.WriteLine(step);
Console.WriteLine($"Memory: {GC.GetTotalMemory(true):N0}");
Console.WriteLine();
} I do not see any issue here. Did you run it in Debug or Release? Have you ran a memory profiler which can demonstrate the leak? public static void Main()
{
const int NUMBER_OF_SUBSCRIBERS = 100_000;
long initial, afterPublish, afterDispose;
var recvrs = new MyReceiver[NUMBER_OF_SUBSCRIBERS];
for (var i = 0; i < NUMBER_OF_SUBSCRIBERS; i++)
{
recvrs[i] = new MyReceiver(i);
}
initial = GetMemory();
Console.WriteLine(nameof(initial) + ": " + initial.ToString("N0"));
Task taskA = Task.Factory.StartNew(() => { MessageHub.Instance.Publish(string.Empty); });
taskA.Wait();
afterPublish = GetMemory();
Console.WriteLine(nameof(afterPublish) + ": " + afterPublish.ToString("N0"));
foreach (var recvr in recvrs)
{
recvr.Dispose();
}
afterDispose = GetMemory();
Console.WriteLine(nameof(afterDispose) + ": " + afterDispose.ToString("N0"));
Console.ReadLine();
}
private static long GetMemory()
{
GC.Collect();
GC.WaitForPendingFinalizers();
return GC.GetTotalMemory(true);
}
private class MyReceiver : IDisposable
{
private readonly Guid _token;
public int Id { get; }
public MyReceiver(int id)
{
_token = MessageHub.Instance.Subscribe<string>(ReceiveMessage);
Id = id;
}
private void ReceiveMessage(string msg)
{
}
public void Dispose() => MessageHub.Instance.Unsubscribe(_token);
} |
What I am trying to demonstrate is that it is holding the objects in memory when published on another thread. For example if you run the same code without publishing in a task you get the following output.
Initial 1st Publish Disposed When you run it in the Task
Initial 1st Publish Disposed As you can see the objects aren't being disposed of when the publish is inside the task. Because the publish and the unsubscribe are on different threads the unsubscribe can't remove the subscription from the [ThreadStatic] _localSubscriptions. |
Yes you are right. The This should now be fixed (2nd time lucky!) in the latest version (v4.0.0). Thanks for reporting this and let me know how it goes. |
I am still seeing a memory issue if you publish twice or more on separate threads before disposing.
Initial 1st Publish 2nd Publish Disposed I believe the issue is just the break in UnRegister. Subscriptions can end up in multiple threads if you publish multiple times before disposing so it should always check all threads instead of stopping after it finds the first match. |
Okay, let's give v4.0.1 a try? |
Looks to all be working now. No longer getting any memory leaks. Thanks for the quick responses! |
Excellent, once again, thank you for reporting. |
The Subscriptions._localSubscriptions variable is populated during Publish, which creates a strong reference to handlers in that array. In the scenario where the thread is kept alive (eg, created on the UI thread), that collection is not updated until Publish is called again.
[ThreadStatic] private static Subscription[] _localSubscriptions;
In my code, that means MessageHub holds my object hierarchy in memory indefinitely, or until the next message is published. Is it possible to remove handlers from the _localSubscriptions during an UnRegister?
The text was updated successfully, but these errors were encountered: