[FullNode] Is disposed 3 times

[Aprogiena]

Described by @noescape00 in https://github.com/stratisproject/StratisBitcoinFullNode/pull/789#issue-276450273

please continue here with the discussion

[noescape00]

what kind of workaround?

By workaround I mean checking if it is already disposed: if (this.IsDisposed) return;

pls show more details about why do you think it is hard to dispose FN just once

FullNode gets disposed 3 times: 3rd time is FullNodeExtensions : 101 (end of the using(node) block)- so here it's all clear and this is intended.

1st and 2nd are called by Microsoft.Extensions.DependencyInjection.ServiceProvider:121 (you need to uncheck Debug=> Options=> Just my code in VS settings to debug that). So on this line we have a _disposables list that for some reason has 2 FullNode items (in total there are 21 and full node ones are the first. I'm not sure if we should dispose FullNode before the services).

First time full node gets added to that list when we call var fullNode = fullNodeServiceProvider.GetService<FullNode>(); at FullNodeBuilder:177

Second time FullNode gets added after we call that: this.ChainBehaviorState = this.Services.ServiceProvider.GetService<ChainState>(); at FullNode:160 The reason why FullNode gets captured here is that ChainState requires IFullNode in the constructor.

So Ideally FullNode should never be disposed by ServiceProvider.

So it's hard to dispose FN once because of how ServiceProvider works. It doesn't give you full control over what should be disposed and what shouldn't. So I guess think it will require changing the source of service provider in order to avoid having it capturing FullNode as Disposable.

[Aprogiena]

ok in theory that ChainState thing should be relatively easy to get rid of, but i'm not if we want that or not it would require providing IBlockDownloadState instead, which should be possible, the interface is trivial, we can have a trivial dummy that does nothing

this way we would get rid of the second dispose

that using in FNE, why it is there? you can see there is Node.Stop which does all the disposing, or not?

[noescape00]

this way we would get rid of the second dispose

Not really. FullNode will be added to disposables in the next service that has IFullNode as a parameter in the constructor. In our case after commenting out this.ChainBehaviorState = this.Services.ServiceProvider.GetService<ChainState>(); FullNode would be added after that call:

FullNode:183 this.fullNodeFeatureExecutor = this.Services.ServiceProvider.GetRequiredService<FullNodeFeatureExecutor>();

So if we really want to avoid adding FullNode to disposables we need to remove reference to full node from every single service's constructor.

that using in FNE, why it is there? you can see there is Node.Stop which does all the disposing, or not?

Yes, I see. I've no idea why it's there.

Though from the logical perspective I think disposing code should be moved from Stop() to Dispose(). Now stop() actually disposes the node but it doesn't set the flag IsDisposed. So potentially we can have node disposed while IsDisposed == false, which is probably not very good.

[Aprogiena]

i see, therefore i've misinterpret your original message

so where do we dispose these guys that are loading into ServiceProvider? can we skip THAT?

[noescape00]

so where do we dispose these guys that are loading into ServiceProvider? can we skip THAT?

FullNode:225 (this.Services.ServiceProvider as IDisposable)?.Dispose();

They get disposed with the ServiceProvider itself. Not sure if we want to skip that.

[Aprogiena]

yes, this line is what i'm asking about because with this line, we don't really control what is disposed when, right? and we should be able to dispose everything manually in a well controlled cascade

[noescape00]

because with this line, we don't really control what is disposed when, right?

Correct. Basically everything that is IDisposable that gets in or out of the ServiceProvider will be disposed when provider gets disposed.

and we should be able to dispose everything manually in a well controlled cascade

Entirely agree. But in order to achieve that we should change how ServiceProvider works. Or, maybe, use some ugly huck like creating our own IDisposable just to bypass this ServiceProvider's line: var disposable = service as IDisposable;

[noescape00]

I think the easiest way would be implementing our own ServiceProvider. Just coping it from Microsoft.Extensions.DependencyInjection and removing 'CaptureDisposable(object service)' would work.

[Aprogiena]

what would be the difference over not calling that dipose line?

[Aprogiena]

i mean what is the problem of not calling that line? what is its purpose?

[noescape00]

what would be the difference over not calling that dipose line?

The only difference would be that if we dont call that line we wont clear ServiceProvider.ResolvedServices dictionary. Not sure about the consequences of that, need to check.

[noescape00]

I guess if the probability of scenario when we need a disposed service after node is stopped is 0 then there is no problem in removing (this.Services.ServiceProvider as IDisposable)?.Dispose(); and doing all the disposing by our own.

Or even better- recreate service provider or set it to null on node disposal so even if this scenario happen target service won't be marked as resolved.

[Aprogiena]

i think we should be ok just by removing that line, would like someone else to confirm though

[Aprogiena]

@fassadlr @dangershony @bokobza anyone of you want to comment on this?

[Aprogiena]

i think if we could check what disposing SP really means we could possibly remove the line if the only thing it does is calling our dispose methods

[dangershony]

Ah right so that is the line that causes dispose on all services, as I said IDisposable should be immutable right? but we want to control our components being disposed.

I would suggest we either remove that line altogether or call it at the very end once we are sure all our controlled components are disposed.

Ah and I see we do call it at the end.

[dangershony]

OK I suggest we remove that lien all together as @Aprogiena suggested

[Aprogiena]

yes, we don't call it, because it would just invoke disposals the only thing i'm not sure about is if all this line does is to invoke disposals of our components and nothing else

if that is true then no problem, we can just remove it and we are done, but we should check that this is the case.

[noescape00]

Yep, it just disposes services in reverse order and clears ResolvedServices list.

Here is the code (Microsoft.Extensions.DependencyInjection.ServiceProvider:106):

public void Dispose()
        {
            lock (ResolvedServices)
            {
                if (_disposeCalled)
                {
                    return;
                }
                _disposeCalled = true;
                if (_disposables != null)
                {
                    for (int i = _disposables.Count - 1; i >= 0; i--)
                    {
                        var disposable = _disposables[i];
                        disposable.Dispose();
                    }
                    _disposables.Clear();
                }
                ResolvedServices.Clear();
            }
        }

[Aprogiena]

so yeah, i guess we don't need to clear resolved services on shutdown so i think we are good to go

[noescape00]

Ok. So after removing this line disposing of services is under features control.

Right now they all dispose their stuff in 'Stop()' method. I don't think this is logical.

I'd suggest doing it using one of following ways: 1) Just rename feature's and node's Stop() to StopAndDispose() 2) Make them implement IDisposable and specifically call Dispose() when we dispose the node.

[noescape00]

Also node has dispose but it calls stop. So the usual scenario is that we shutdown the node by calling Stop() and then we would dispose it but it was disposed using Stop even though 'this.IsDisposed' flag is still false. I'd really like to move disposing code to Dispose() or get rid of IDisposable implementation at all.

[Aprogiena]

what you say is basically what .NET core is now doing everywhere. before you often had these Close() methods in .NET 4 and then they removed them in favor of one single Dispose method. so what you say is a modern trend i'd say :-)

Therefore I very much agree with you and then the only question is whether it is in or out of scope of this PR. As we discussed previously, we want rather smaller PRs. This sounds like that the PR would gain in its size. And possibly, these things are separated things, or not?

Any comments @dangershony ?

[noescape00]

As we discussed previously, we want rather smaller PRs.

Well, this PR is a one liner so I can submit it and create a new one for changing how we approach disposing.

[noescape00]

these things are separated things, or not?

Yes. They are pretty much independent.

[Aprogiena]

in this PR i think we want to make sure that

• everything is disposed no more than once
• everything is disposed at least once

in all FN code, not in NBitcoin so possibly we will find more undisposed things (i found some today in recently ported NBitcoin code)

and esp. we want to make sure FullNode is disposed 1x not 3x and ConnectionManager is diposed 1x not 3x

[noescape00]

That one wrong dispose call for CM was because feature that didn't add CM to services disposed it.

What do you think about a general rule that if service was added by feature's builder extension it can be disposed only from this feature's stop\dispose code and not from any other random feature?