Queuing e-mail notifications in a background thread

Question

I'm new to parallel programming concepts. I'm trying to implement fire-and-forgot kind of method in my project. Somebody please evaluate the below code & let me whether this is thread safe or not.

 //Controller
 public ActionResult AddMail(Guid userId)
 {
 EmailNotification.QueueMailAync(userId);
 }
 //BL
 public static class EmailNotification
 {
 public static void QueueMailAync(Guid userId)
 {
 HostingEnvironment.QueueBackgroundWorkItem
 (
 cancellationToken =>
 {
 try
 {
 QueueMail(userId);
 }
 catch (Exception e)
 {
 logger.Error(e);
 }
 }
 );
 }
 static void QueueMail(Guid userId)
 {
 using (var context = new DBEntities())
 {
 var user = context.Users.FirstOrDefault(u => u.Id == userId);
 string body = context.EmailContents.FirstOrDefault().BodyText;
 body = body.Replace("{USER_NAME}",user.UserName);
 var mail = new ArchOver.Model.Email();
 mail.BodyText = body;
 mail.UserId = userId;
 mail.Subject = "Aync";
 mail.EmailTo = user.Person.Email;
 context.Users.Add(mail);
 context.SaveChanges();
 }
 }
}

• Nikola.Lukovic: Read these blogs from our lord and savior Eric Lippert Link1 Link2 Link3

Answer 1

I would always advocate SingleOrDefault() over FirstOrDefault() if the intent of the query is to only expect 0..1 data objects returned.

And to explain further.....

My comments are mainly around the use of FirstOrDefault(). To me your code isn't wrong as such and the use of FirstOrDefault will produce what you are looking for.

However, there are two problems I see with it.

Intent

When I read FirstOrDefault() I read that to mean that there could potentially be multiple items (0..n) returned from a query. So that means there is a business reason behind this decision and I would expect the rest of the application to be able to adhere and follow this decision without problems.

So in your case I read this line

context.Users.FirstOrDefault(u => u.Id == userId)

to mean something along the lines of

There might potentially be more than one user in the database with an Id that matches the userId supplied. However I'm only interested in the first one so I will ignore all other users.

Masking potential problems

Using FirstOrDefault() from past experience is typically used because developers don't want an exception to be thrown if the criteria constraint is violated i.e. they don't want to see a bug come through.

However this may also mean that an error occurs somewhere further down stream because of this. You will then find yourself debugging errors that in fact relate to model data violations and might have nothing to do with the code you are debugging.

The problem should ideally be fixed at the point where the model data violations were made, rather than the rest of the application having to worry about it.

Summary

I would always advocate SingleOrDefault() over FirstOrDefault() if the intent of the query is to only expect 0..1 data objects returned.

So use what you expect the data state to be in. Unless of course you want to handle bad data state yourself and then I would expect error handling to occur if this was violated.

• Pierre Menard: I was shaking my head reading this until I got all the way to the Summary - that's the first time you mention SingleOrDefault. Your answer would be clearer if you propose the alternative earlier.
• dreza: @PierreMenard Lucky you read through the whole answer then eh :)
• CodesInChaos: The code doesn't even handle the null case, so all OrDefault variants are wrong.
• dreza: @CodesInChaos yes indeed you are correct in that regard

Answer 2

Thread safety is about 2 different threads simultaneously accessing/modifying the same resource. By their nature, static classes are inherently shared. But in your case the class has no member variables. So there aren't any data objects being shared. So your class isn't creating any problems, but it could be propagating problems that exist in other places. For instance, there isn't a lock around HostingEnvironment.QueueBackgroundWorkItem, so multiple threads could be calling into it. But that's ok, it's designed for that. What about QueueMail? Each invocation instantiated a different DBEntities object and Email object to work with. You should check the documentation for those classes, but the general assumption is that it's not necessary to synchronize calls to (non-static) member functions of different instances.

In short, your code looks thread-safe to me.