orleans icon indicating copy to clipboard operation
orleans copied to clipboard

Should ActivationData Scope be dispose explicitly in all scenario?

Open EranOfer opened this issue 5 years ago • 5 comments

I notice that when an exception is thrown during grain OnActivateAsync /constructor the catalog doesn't dispose of the ActivationData scope.

    internal class ActivationData : IGrainActivationContext, IActivationData, IInvokable, IDisposable
{
......


.....
      internal void SetupContext(GrainTypeData typeData, IServiceProvider grainServices)
        {
            this.GrainTypeData = typeData;
            this.Items = new Dictionary<object, object>();
            this.serviceScope = grainServices.CreateScope();
public interface IServiceScope : IDisposable

The Dispose() method ends the scope lifetime. Once Dispose is called, any scoped services that have been resolved from ServiceProvider will be disposed.

In the default implantation, there is no problem due to the fact the ServiceScope is the only one who references the scope object.

image

But it depends on the implementation of the IServiceScope I encounter other implantation that must dispose of the scope or it causes a memory leak.

I think we should always dispose of the scope.

EranOfer avatar Feb 26 '20 07:02 EranOfer

Yes, it should. If a scope is created then we must dispose it. Thank you for investigating!

Are you able to open a PR to fix this?

ReubenBond avatar Feb 27 '20 00:02 ReubenBond

I investigated a memory leak, I decided that the best approach for me is to create other implementation of the service scope eliminating this kind of leaks in the first place.

I don't sure, I have time to create a PR.

The problematic flow:

sequenceDiagram
    catalog-->>ActivationData:GetOrCreateActivation
    ActivationData-->>ActivationData:constractor
    catalog-->>catalog:RegisterMessageTarget(ActivationData )
    catalog->>catalog:activations.Add
    catalog-->>catalog:SetupActivationInstance
    catalog-->>ActivationData:SetupContext() !!
    catalog-->>catalog:CreateGrain<throw>
    catalog-->>catalog:UnregisterMessageTarget
    catalog-->>catalog:activations.Remove
    ActivationData-->ActivationData:Missing Dispose like heppand in the FinishDestroyActivations

I missing a lot of context but ,my assumption is that on UnregisterMessageTarget that grain should be dead and can safly dispose of the context .

EranOfer avatar Feb 27 '20 04:02 EranOfer

I think the scope is not disposed also a case of duplicate activation.

EranOfer avatar Mar 01 '20 07:03 EranOfer

@ReubenBond, I read the code, I am trying to understand what can affect failing to create the state due to duplicate activation. As I understand from the code fail to create due to duplicate activation race is affected by a few conditions.

  • Time to publish grain directory.
  • Placement strategy (local)
  • Time to create the Grain.
    • OnActivateAsync
    • Constructor

What should be the approach to avoid failers to create the grain due to duplicate activation when your grain needs to do long init operation (what is the right stage)?

EranOfer avatar Mar 01 '20 10:03 EranOfer

We've moved this issue to the Backlog. This means that it is not going to be worked on for the coming release. We review items in the backlog at the end of each milestone/release and depending on the team's priority we may reconsider this issue for the following milestone.

ghost avatar Jul 28 '22 23:07 ghost

ActivationData was rewritten in 7.0 and if there was an issue here, it was likely fixed along with that. I'll close this now but please open a new issue if you still experience this issue

ReubenBond avatar Dec 11 '23 03:12 ReubenBond