StratisBitcoinFullNode icon indicating copy to clipboard operation
StratisBitcoinFullNode copied to clipboard

Ensure disposing methods doesn't throw without handling the disposal of inner disposable objects

Open MithrilMan opened this issue 6 years ago • 0 comments

We must be careful when implementing a Dispose() method or when we need to dispose inner disposable objects, because if we don't handle a potential throw, we may end up having disposable object not being able to be collected, or worse.

as an example, assume we have something like this:

public AsyncExecutionEvent<INetworkPeer, NetworkPeerState> StateChanged { get; private set; }
public AsyncExecutionEvent<INetworkPeer, IncomingMessage> MessageReceived { get; private set; }

where the class AsyncExecutionEvent is disposable and then we dispose manually. Now I'm not saying that we have problem in this specific code, but that if the Dispose method of the parent that should call MessageReceived.Dispose() fails, it will never been disposed.

I think as a first step we should trap all the Dispose() method of containers in such a way that in the catch we can dump a log.error that's important to see, so we can fix eventually the code

something like

///this is the Dispose of the container of other disposable objects
public void Dispose()
{
   try
   {
       //do your disposing stuffs, calling MessageReceived.Dispose()  or whatever
   }
   catch
   {
       log.Critical("DAMN, fix this");
      throw
   }
}

would be better maybe to have a kind of registration process for disposable resources, so we can add a finally in the try/catch block that check if we disposed all the disposable we had to dispose.

MithrilMan avatar Jan 04 '19 18:01 MithrilMan