2

I'm using AutoFac to inject a concrete data context in my web application. I want to execute the database context's SaveChanges() method at the end of the request if there were no exceptions on the page. Otherwise I just want to dispose of the context as normal.

I noticed that AutoFac has an OnRelease method. Intellisense for the method states:

Run a supplied action instead of disposing instances when they're no longer required.

As such, I was thinking of doing something like this:

builder.RegisterType<MyContext>().As<IDbContext>().InstancePerHttpRequest()
    .OnRelease(x => {
        if (HttpContext.Current != null && HttpContext.Current.Error == null)
            x.SaveChanges();
        if (x != null)
        {
           x.Dispose();
           x = null;
        }
     });

Is this an appropriate place to commit changes for the data context? Is it guaranteed to run on every request, even when an Exception occurs?

Sam
  • 9,933
  • 12
  • 68
  • 104

2 Answers2

2

In general, I don't like the approach when you save changes on request end because you lose flexibility here. The good practice is to save changes on business transaction end. Imagine this sample code:

public ActionResult CreateAccount(CreateAccountModel createAccountModel)
{
    // Your business transaction start here from validating and processing input data
    var account = CreateAccountFrom(createAccountModel);
    _yourContext.Add(account);
    // Your business transaction ends here
    // This is the good place to commit your transaction
    _yourContext.SaveChanges();

    // You can have another business transaction here

    // the following is not important code to log the event
    // which could break the business transaction if it would be within one
    // you can wrap it in try-catch for example
    _yourContext.Add(new Event(){ Type = AccountCreated });
    _yourContext.SaveChanges();

    // code to retrieve date for the view
    var viewModel = GetViewModel();
    return View(viewModel);
}


Now regarding your code, in short, it's a bad place to save changes. First of all you violate Single Responsibility principle, OnRelease supposed to clean resources on classes without IDisposable, but not to do additional logic. It's bad to put business logic there only because you can do it. The second thing, If you get an exception on x.SaveChanges() your context won't be disposed. It's better to not mess the business logic and object lifetime logic.

Alexandr Nikitin
  • 7,258
  • 2
  • 34
  • 42
  • Alexandr, thanks for your answer. I do have the flexibility to call `SaveChanges()` within my code when needed. Also, couldn't I wrap the `SaveChanges()` in a try/catch/finally block just like I would in the rest of my code to handle your worries about disposal? – Sam Jan 22 '14 at 13:13
  • 1
    @Sam "I do have the flexibility to call SaveChanges() within my code when needed." Why do you want to combine this two approaches? It leads to bugs. "couldn't I wrap the SaveChanges() in a try/catch/finally block" Yes, it will work but... :) – Alexandr Nikitin Jan 22 '14 at 16:05
0

I agree with Alexandr's conclusion that it's a bad place, but I disagree with his reasoning behind it.

My reasoning for not wanting to do that is because your request will end and return a 200 response. OnRelease will get called presumably on another thread and if it fails, you're out of luck status code wise.

EF inherently implements the unit of work pattern, so IMO you don't want to call save changes wherever you want. At the end of the request, saving once should be sufficient.

I wouldn't call it on the controller method either, especially if most or all of your controllers access your dbcontext. Instead you can create an action filter and wire it up globally.

Here's a .net core example:

public class SaveChangesFilter : IActionFilter
{
    private readonly ILogger<SaveChangesFilter> _logger;
    private readonly DbContext _dbContext;

    public SaveChangesFilter(IDbContext dbContext, ILogger<SaveChangesFilter> logger)
    {
        _logger = logger;
        _dbContext = dbContext as DbContext;
    }

    public void OnActionExecuting(ActionExecutingContext context)
    {
    }

    public void OnActionExecuted(ActionExecutedContext context)
    {
        if (_dbContext.ChangeTracker.HasChanges())
        {
            try
            {
                _dbContext.SaveChanges();
            }
            catch (Exception e)
            {
                context.Result = <insert error response here>

                _logger.LogCritical(e, e.Message);
            }
            finally
            {
                _dbContext.Dispose();
            }
        }
    }
}

Then in the app startup ConfigureServices method:

services.AddMvc(options =>
                {
                    options.Filters.Add<SaveChangesFilter>();
                })
sdrevk
  • 1,703
  • 2
  • 13
  • 10