Retry failed emails #6312
Retry failed emails #6312
Conversation
zivkan
requested review from
joelverhagen,
agr,
loic-sharma,
xavierdecoster,
shishirx34,
ryuyu,
chenriksson,
cristinamanum,
skofman1,
scottbommarito and
zhhyu
| sentMessage = false; | ||
| } | ||
|
|
||
| private ErrorLog errorLog; |
There was a problem hiding this comment.
coding conventions: fields are first in the class, private fields start with underscore, we don't use "this."
| :base(mailSender, config, telemetryService) | ||
| { | ||
| this.errorLog = errorLog; | ||
| this.messageServiceFactory = messageServiceFactory; |
|
How did you test this? |
| } | ||
|
|
||
| private ErrorLog errorLog; | ||
| private BackgroundMessageServiceFactory messageServiceFactory; | ||
| private bool sentMessage; |
There was a problem hiding this comment.
private fields should be prefixed with _
e.g. _sentMessage
| :base(mailSender, config, telemetryService) | ||
| { | ||
| this.errorLog = errorLog; | ||
| this.messageServiceFactory = messageServiceFactory; |
There was a problem hiding this comment.
if you prefix the private fields with _ you don't need the this.
|
|
||
| namespace NuGetGallery.Services | ||
| { | ||
| public class BackgroundMessageServiceFactory : IBackgroundMessageServiceFactory |
There was a problem hiding this comment.
I think Autofac is smart enough that you can replace this class with Func<IMessageService> and it will return a function that does the same thing as this.
| var messageCopy = CloneMessage(mailMessage); | ||
|
|
||
| Task.Run(async () => | ||
| if (sentMessage) |
There was a problem hiding this comment.
It seems really odd to have an object that makes clones of itself to perform its primary purpose.
I think it's pretty confusing that the "BackgroundMessageService" is really just the "BackgroundSingleMessageService".
We also lose any reference to the additional IMessageServices created to send mails, so there's no visibility on how many mails are sent or how many instances of this class there are created by a single request.
I would rather have
- a class that sends emails (the section of this method inside the
else) - a class that has a method that sends emails by creating an instance of the class that sends emails
The big issue here is that we have one class (IMessageService) that creates and sends emails instead of one class for each of those purposes. If the class was split, it would improve the structure of this code, but I think that's outside the scope of this change.
Given that caveat, it may be better if additional emails sent during a single request waited for previous emails to finish, but I'm with how you have it here as well.
zivkan
force-pushed
the
zivkan/retry-send-emails
branch
from
a1f6a8a
to
b108cef
Compare
|
Testing was done manually by changing the SmtpUri to smtps://user:passwrd@localhost:25/, with nothing listening on the port so all attempts will fail with a network error. Putting a breakpoint on BackgroundMessageService's background task's exception handler, without the last commit, I could see many exceptions were about an async operation is already underway. Checking App Insights telemetry shows the smtp dependency failed. With the changes, the exception breakpoint only sees network related exceptions. This morning I tried setting SmtpUri to a valid SMTP server, and using the contact owner functionality with , I got two emails. In reply to: 413279980 [](ancestors = 413279980) |
This reverts commit 9b00ef8.
Sending emails in a background thread now means any MVC action that sends multiple emails will be sending them concurrently. This change ensures that each message gets its own SmtpClient. Previously, after the first email, the SmtpClient would throw an exception saying that an Async operation is already in progress.
zivkan
force-pushed
the
zivkan/retry-send-emails
branch
from
b108cef
to
488f43e
Compare
The first commit is just a revert of what was previously reviewed and merged in dev. So, you can save yourself some time by just looking at the second commit's changes.
The problem with the previous PR that needed to be reverted was that previously emails are sent synchronously, so only one email being sent at a time. In order to prevent users waiting a long time for HTTP responses, emails now are sent in the background. However, when a single MVC action sends two or more messages, only the first would be sent and the others would throw an exception because the SmtpClient couldn't start sending a new message while an async operation was already in progress.
This PR fixes that issue by creating a BackgroundMessageServiceFactory. When an instance of BackgroundMessageService detects that it is being reused, it calls the factory to create a new instance, and sends the new email using the new instance.