1

Consider this code:

public async Task<Status> SendMessage(Message message) { List<IMessage> _messageDispatchers = new List<IMessage>(); try { Object[] args = new Object[] { _message }; IMessage endpoint = (IMessage)Activator.CreateInstance(Type.GetType(_message.AgentDLLName), args); _messageDispatchers.Add(endpoint); foreach (IMessage dispatcher in _messageDispatchers) { await Task.Run(() => dispatcher.SendMessage(_message)); } return await Task.Run(() => Status.Success); } catch (Exception ex) { logger.Log(LoggerLevel.Error, ex.Message); return Status.EmailSendingFailed; } }

the SendMessage:

public async Task<Status> SendMessage(OutboundMessage outboundmessage) { string strMessage = string.Empty; string subject = string.Empty; MessageServices objService = new MessageServices(); try { var config = (from SmtpConfigurationElement ms in AppConfiguration.Instance.Smtps where ms.Key == "smtp" select ms).Single(); SmtpClient smtpClient = new SmtpClient(config.Host); smtpClient.Port = Convert.ToInt32(config.port); smtpClient.EnableSsl = true; smtpClient.Credentials = new NetworkCredential(config.UserName, config.Password); string[] strToList = outboundmessage.ToList.Split(';'); MailMessage mail = new MailMessage(); mail.From = new MailAddress(outboundmessage.FromAddress); if (strToList.Length > 0) { for (int j = 0; j < strToList.Length; j++) { mail.To.Add(strToList[j]); } } else { _LOGGER.Log(LoggerLevel.Information, "SMTP Mail Send failed as ToList is not correct"); return Status.Failed; } if (!string.IsNullOrEmpty(outboundmessage.CCList)) { string[] strCCList = outboundmessage.CCList.Split(';'); if (strCCList.Length > 0) { for (int k = 0; k < strCCList.Length; k++) { mail.CC.Add(strToList[k]); } } } if (!string.IsNullOrEmpty(outboundmessage.Attachments)) { System.Net.Mail.Attachment attachment; attachment = new System.Net.Mail.Attachment(outboundmessage.Attachments); mail.Attachments.Add(attachment); } strMessage = await objService.ReplaceMessageWithPlaceholders(outboundmessage.PlaceholderValues, outboundmessage.MessageBody); subject = await objService.ReplaceMessageWithPlaceholders(outboundmessage.PlaceholderValues, outboundmessage.Subject); mail.Body = strMessage; mail.Subject = subject; mail.IsBodyHtml = true; await Task.Run(() => smtpClient.Send(mail)); return Status.Success; } catch (Exception ex) { return Status.Failed; } }

And the call to SendMessage:

public Status MarketingEmail(OutboundMessage _message) { try { _message.MessageCreatedDate = System.DateTime.Now; processor.SendMessage(_message); return Status.Success; } catch (Exception ex) { _LOGGER.Log(LoggerLevel.Error, "Error in Marketing Email" + ex.ToString()); return Status.InsertFailed; } }

The whole idea is to make a workflow in which sending of the email is the last task and that should be a fire and forget thing.

Now the call to processor.SendMessage(_message) has a suggestion like this:

Because this call is not awaited, execution of the current method continues before the call is completed. Consider applying the 'await' operator to the result of the call.

Which is a valid thing since async & await need to be used together.

Questions:

  1. Will the current approach work without any trouble if the suggestion is ignored? (I am asking this since this is still in the development stage and I can make the suggested design changes now rather than face any critical issues later.)
  2. What is the suggested best practice to design a workflow considering the said requirement?
Improve this question
1
  • is Status.Success a enum? if it is you should not be doing Task.Run(() => Status.Success) do return Task.FromResult(Status.Success) or just return Status.Success; as approprate, no need to start up a task and wait for it. Commented Oct 11, 2014 at 5:04

1 Answer 1

Reset to default
5

The current approach will "work" in the sense that it will continue on to return Status.Success; without waiting for the call to processor.SendMessage(_message); to complete.

However, since that call was fired & forgotten, and that SendMessage overload doesn't do any logging in the catch block, you run the risk of emails failing to be sent but nobody getting notified about it.

A common approach for async email sending is this: Stash the email somewhere else (typically a message queue or a database), and then set up a separate async process that reads the queued emails and sends them. If it succeeds, it flags the email as sent. If it fails, it tries again (up to a certain time limit or # of retries), and then if it gives up, it can trigger a notification or set a flag that can be checked later.

Then your code will basically be saying "okay, the email was successfully queued", instead of "okay, the email was sent". Moving the actual sending to a separate process is much more reliable.

Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

P.S. If you go to implement the email queuing approach, you may be tempted to serialize the MailMessage as JSON and save it that way. But it won't deserialize back to a MailMessage instance, because the MailAddress class, which MailMessage uses internally, doesn't have a parameterless constructor. There are ways around that, but we found it easier to just save the email to a database table with discrete columns for Subject, From, To, CC, BCC, Body, DateCreated, DateSent, and so on. Hope this helps.
This answer is correct, but not clear when "but nobody getting notified about it" is said. The technical reason is that without await .NET CLR won't pass the exception (if any) back to the caller thread, and the exception drops silently. Thus, the C# compiler generates a warning to warn the developer explicitly.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.