Is it a good practice to Execute API on Dispose?

Question

I have some legacy code that execute an Http API call on Dispose, this class read some data from an API and permits the manipulation, the save of the manipulated data is executed in the Dispose method calling an API.

Example "pseudo" code:

public Class Manipulator : IDisposable
{
    public Manipulator(){}
    public void Load(int ID){ ... call API to load data ... }
    public void Manipulate1 { ... manipulate data locally ...}
    ...
    public void ManipulateN { ... manipulate data locally ...}
    public void Save(){ ... call API to save data ... }

    protected override void Dispose(bool disposing)
    {
            if (!_disposed)
            {
                if (disposing)
                {
                    Save(); // Call and Http API to save the data
                }
                _disposed = true;
            }
        }
    }
}

and the code is used as:

using(var data = new Manipulator())
{
    data.load(123);
    data.manipulate(...)
}// Saved on dispose

Ignoring exception that are intercepted etc... Is this an acceptable pattern? or it is considered a bad practice? alternatives?

It seems like it only solves half the problem, anyway. You are "required" to call `Save`, but nothing's forcing you to call `Load`, making it quite easy to store inconsistent data. I would be opposed to this for no other reason than that properly handling exceptions raised in a `Dispose` is difficult if not impossible, and `Dispose` implementations should therefore do their utmost to not do things that can cause errors that need to be recovered from. Calling an HTTP API is therefore right out, as this *can* be expected to fail, and *would* need to be recovered from. — Jeroen Mostert, Mar 14 '23 at 10:00
If not calling `Save` is to be considered a programming error, the `Dispose` could also be made to just check if `Save` has been called, and if not produce an error. This error would not be expected to be recoverable, so it's less of an issue. Alternatively it could just log the condition somewhere, in a way that can't fail. Another option is to have a custom analyzer rule verify that no instance of `Manipulator` goes out of scope without `Save` having been called, handling the problem before it can happen at runtime. — Jeroen Mostert, Mar 14 '23 at 10:08
If a type implements `IDisposable` it's common to use it in a `using`-block. That forces the call to `Dispose` and in your case it saves the data. The problem is that you can't avoid that saving operation. What if you've an instance, manipulate it but for whatever reason you don't want to save that changes? The `using`-block will call it unless you set any property that is handled in `Dispose`. That sounds wired. — Sebastian Schumann, Mar 14 '23 at 10:13

score 1 · Answer 1 · answered Mar 14 '23 at 11:23

No, this is not a good idea.

First of all, interfaces exist to describe behavior. IDisposable is documented as:

Provides a mechanism for releasing unmanaged resources.

That's not what it's being used for here. See also Eric Lippert in "Is it abusive to use IDisposable and "using" as a means for getting "scoped behavior" for exception safety?":

I expect that "using" is used to use a resource and dispose of it when you're done with it. Changing program state is not using a resource and changing it back is not disposing anything. Therefore, "using" to mutate and restore state is an abuse; the code is misleading to the casual reader.

So you're doing something weird that nobody else using that interface does. Apart from the ASP.NET MVC team with their using @Html.BeginForm() {} but that's old news now since the advent of Core as well.

Finally, what if an exception occurs that causes your model to not be API-saveable? The disposing code will call the API nonetheless.

Just don't do this. Call Save() explicitly.

Sebastian Schumann · Answer 2 · 2023-03-14T11:13:11.407

I would not call save operations in Dispose because there could be various reasons that you might not want to save the changed state after all.

But that doesn't mean that it is always a bad idea to move code to Dispose.

If you want to make absolutely sure that the code is executed no matter what, a Dispose with a using-block is a good idea.

Lets say you have something like BeginUpdate and EndUpdate or unregistering and reregistering to events or disabling and enabling of controls. It's good practice to set a certain condition before entering a try..finally and clean up in finally.

A sample code might look like:

this.control.BeginUpdate();
try
{
    // Do stuff in e.g. more than 20 lines here.
}
finally
{
    this.control.EndUpdate();
}

Yes you might say that you should move that amount of code into a separate method. But such code exists.

The code that creates a condition is quite far away from the code that cleans up again.

Let's say the code will look like that:

this.control.BeginUpdate();
using var @finally = new Finally(this.control.EndUpdate);

// Do stuff in e.g. more than 20 lines here.

Anyone who sees the code will immediately see that once established, conditions are cleaned up correctly - right on the next line of code. Please keep in mind that the next line of code only enforces that the cleanup code is called at the end of the surrounding block. It does not call EndUpdate immediately after BeginUpdate. EndUpdate is called at the end of the block.

For that you need a struct like:

public readonly struct Finally : IDisposable
{
    private readonly Action? _onDispose;

    public Finally(Action onDispose)
    {
        ArgumentNullException.ThrowIfNull(onDispose);
        
        this._onDispose = onDispose;
    }

    public void Dispose() => this._onDispose?.Invoke();
}

Even Stephen Cleary uses this way in Nito.AsyncEx for e.g. Semaphore.

The code for acquire and releasing a lock might looks like:

using var lockHandle = this._sem.Lock();

A `using` statement (so not a block) extends to the end of the *scope*, not "the next line of code". This typically encompasses *more* statements than a block, not less. One must also be careful about statements like code being executed "no matter what" -- `Dispose` is *only* invoked if client code invokes it, explicitly or through `using`. There are many circumstances where it won't run; the primary benefit is that it's harder to accidentally omit and that analyzers can catch it if you do, in contrast to a `finally` block. — Jeroen Mostert, Mar 14 '23 at 10:49
@JeroenMostert _A using statement (so not a block) extends to the end of the scope, not "the next line of code"._ Yes, I did not claim anything else. — Sebastian Schumann, Mar 14 '23 at 10:58

Is it a good practice to Execute API on Dispose?

2 Answers2