Async & ContinueWith

Question

I am implementing my own UnitOfWork for an EntityFramework-DataContext.

I want to call SaveChangesAsync and when this succeeds, clear a list. Is my approach with ContinueWith correct?

public async Task<int> CommitAsync(int? commandTimeout = null, bool ignoreCustomLogic = false)
{
 try
 {
 return await DataContext.SaveChangesAsync().ContinueWith((antecedent) =>
 {
 if (!antecedent.IsFaulted)
 {
 _addedEntities.Clear();
 _changedEntities.Clear();
 _deletedEntities.Clear();
 }
 return antecedent.Result;
 });
 }
 catch (DbEntityValidationException dbEx)
 {
 RethrowException(dbEx);
 return default(int);
 }
}
private void RethrowException(DbEntityValidationException dbEx)
{
 string message = "ValidationExceptions: " + Environment.NewLine;
 foreach (var validationResult in dbEx.EntityValidationErrors)
 {
 foreach (var validationError in validationResult.ValidationErrors)
 {
 message += string.Format("Property: {0} Error: {1}{2}", validationError.PropertyName, validationError.ErrorMessage, Environment.NewLine);
 }
 }
 throw new Exception(message, dbEx);
}

• psaxton: @mjolka Posted as answer
• mjolka: I'm rolling this back to revision 4, since the changes to the code have invalidated @psaxton's answer. Please see What you may and may not do after receiving answers.

Answer 1

I want to call SaveChangesAsync and when this succeeds, clear a list.

You don't need ContinueWith, you can just write it like this

public async Task<int> CommitAsync()
{
 var result = await DataContext.SaveChangesAsync();
 _addedEntities.Clear();
 _changedEntities.Clear();
 _deletedEntities.Clear();
 return result;
}

In the original code, the catch block will never run.

Consider what happens when SaveChangesAsync throws a DbEntityValidationException: antecedent.Result will throw an AggregateException wrapping the DbEntityValidationException.

It's poor practice to throw an Exception as it means client code cannot be specific about the exceptions it catches. I would suggest changing RethrowException to a method that constructs the exception message you want, then calling it like this:

public async Task<int> CommitAsync()
{
 try
 {
 var result = await DataContext.SaveChangesAsync();
 _addedEntities.Clear();
 _changedEntities.Clear();
 _deletedEntities.Clear();
 return result;
 }
 catch (DbEntityValidationException e)
 {
 var exceptionMessage = GetDbEntityValidationExceptionMessage(e);
 throw new DbEntityValidationException(exceptionMessage, e);
 }
}

This also removes the return default(int); which is just there to appease the compiler.

• Nick Udell: Alternatively, you don't need the await keyword. One or the other could be removed. I prefer mjolka's method, as it is much easier to read.
• psaxton: Your refactor does not take the case of antecedent.IsFaulted into consideration like the original code did.
• mjolka: @psaxton antecedent is either faulted, cancelled, or ran successfully to completion. Since we're not passing a CancellationToken to SaveChangesAsync, I don't see how it can be cancelled. So the lists will only be cleared in the original code if the SaveChangesAsync ran successfully to completion -- which is the same in the code I posted.
• psaxton: I suppose that makes sense. I've not used ContinueWith before and was not aware of that; I just see that the original is checking for IsFaulted and catching as well.

Answer 2

Something @mjolka corrected but didn't comment on was the unused parameters in your SaveChangesAsync method signature.

If method parameters are not used, don't add them. This is an example of the YAGNI principle. If you later implement the functionality you anticipate using the parameters for, you have locked yourself into an implementation which may not fit your future needs. You also risk breaking consumers expecting the current behavior.

If unused parameters are needed to satisfy an interface definition, seriously consider if your object fits the intent of the interface. This is commonly considered design by contract. Otherwise, implement the method to satisfy the interface contract. As a last resort, document the discrepancy in XmlDoc comments if the the object satisfies the intent of the interface and a proper implementation is not reasonable. In your example, commandTimeout should be implemented. A consumer relying on proper behavior could have disastrous consequences. There doesn't seem to be any reasonable custom logic possible in this implementation -- indicate ignoreCustomLogic is ignored if it can't be removed entirely.

• user2900970: I edited my post. The parameters are not unused but not part of my question. I had the intension to remove them, but forgot to do so...