Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sending to multiple recipients via SmtpClient fails entirely if any of the RCPT commands result in errors. #309

Closed
NathanLBCooper opened this issue Mar 10, 2016 · 21 comments
Labels
enhancement New feature or request question A question about how to do something

Comments

@NathanLBCooper
Copy link

Let's say I use SmtpClient to deliver mail to two addresses disabled@foo.com and valid@foo.com. The first address causes the downstream server to return a SmtpStatusCode.MailboxUnavailable and the second address is just fine (SmtpStatusCode.Ok).

The method ProcessRcptToResponse will throw and noone will recieve any email. As the caller I will be able to see the exception (SmtpCommandException (SmtpErrorCode.RecipientNotAccepted, response.StatusCode, mailbox, response.Response)) and I'll be able to infer that a email to disabled@foo.com won't work.

My objectives are: 1) to send the message to whoever I can. 2) To know who I can't send messages to (so I can make bounce messages etc).

I notice that your implementation decision was to ditch the whole SMTP conversation if one RcptTo fails. Do you recommend any approach or pattern for callers who want to make a best effort attempt to send the email to as many recipients as possible?

I've thought about the approach of subclassing and overriding ProcessRcptToResponse to raise events and supress exceptions, but in in the event of everything failing continuing on to the Bdat and Data isn't correct. I can't see a non-hacky way to get this control flow right.

I have also considered, outside the SmtpClient, stripping the recipients out, in turn, as they fail. But that doesn't seem like a good solution either,

@jstedfast
Copy link
Owner

As of, I think, MailKit 1.2.20, I added a Verify(MailboxAddress) method to SmtpClient that can be used to check whether or not the SMTP server will accept an email address.

Unfortunately, this method is not 100% reliable because some servers always return "command not implemented" (such as comcast) and some always return a status code saying that they don't know if it exists, but that they will attempt delivery (such as gmail). Typically when a server returns "will attempt delivery", it means they'll accept the address in a RCPT TO command just fine.

So depending on your SMTP server, this may or not not be useful for your purpose - I do think it's worth looking into, though, because it'll be a lot more efficient than calling Send() and having it fail and then removing addresses until it finally goes through.

My intended work-around for this problem was to simply catch SmtpCommandExceptions and remove the recipient addresses as they were found to be unavailable/etc.

Of course, when doing this, I would recommend using the Send() method that takes an explicit sender and list of recipients as opposed to just the MimeMessage so that you don't actually have to modify the message itself on fail, but I suppose that all depends on your use case.

@jstedfast jstedfast added the question A question about how to do something label Mar 10, 2016
@jstedfast
Copy link
Owner

If you'd like to go the route of overriding ProcessRcptToResponse(), you could override the Send() method that you are using and have it store the list of recipients on your custom SmtpClient so that you could access that list and see if it's the last address in the list or not, thereby allowing you to throw an exception if none of the addresses were accepted.

@jstedfast
Copy link
Owner

would it be helpful to pass the MimeMessage to the ProcessRcptToResponse() method? Perhaps I can alter the API to do that if that would make it easier for you to achieve your goal.

@NathanLBCooper
Copy link
Author

Thank you very much for your reply. I'll give this some thought and, if I find a good solution, I'll add some comments here.

@RoyTinker
Copy link

@NathanLBCooper, in the referenced issue (#256), @jstedfast provided the ability to override ProcessRcptToResponse, and that has been sufficient for my use case (which is to not throw an exception if UserNotLocalTryAlternatePath, MailboxNameNotAllowed, or MailboxUnavailable is returned -- instead notify the team).

Just imagining here -- I might be mistaken/ignorant, so bear with me -- if (and only if) the emails you're sending do not contain private user data, perhaps the ideal solution would be to use a single dedicated, internal address for RCPT TO in the SMTP session (which would also appear in the MimeMessage's CC list) -- and then let the MimeMessage's To/CC lists handle the true, intended destinations. In that case, even if all addresses fail, the message would still be sent (to your internal address) and you'd have record of it. Also, overriding ProcessRcptToResponse and not throwing or catching thrown exceptions for the above 3 responses would be sufficient (or even unnecessary)... although specifying a CC'd address as RCPT TO may require other changes, or may not be possible at all. Just an idea.

@RoyTinker
Copy link

I can't remember... but does MailKit send RCPT TO for each To and Cc address, or only the first one? As far as I remember (and I may be mistaken), SMTP requires only one RCPT TO address specified, even if the transmitted MIME content specifies more To, Cc, and Bcc addresses.

@jstedfast
Copy link
Owner

It sends a RCPT TO for each To, Cc and Bcc address.

@RoyTinker
Copy link

Interesting. Is it considered bad form (or using RFC verbage, "clients SHOULD NOT") to have addresses in the MimeMessage that weren't sent as RCPT TO during the SMTP session?

@jstedfast
Copy link
Owner

It's not really bad form, in fact it's how clients can send blind carbon copies (the message you send has the Bcc header removed).

To be honest, I think the MAIL FROM and RCPT TO commands are an artifact of how messages were sent before MIME and probably before the "Internet Message" format was solidified, because they could easily have parsed the headers and figured out who to send the message to, but I guess convention made it such that the SMTP server wouldn't have to.

@RoyTinker
Copy link

So if a recipient isn't added to the SMTP envelope, they won't receive the message regardless of the MimeMessage contents... Oh well, in that case my idea is (partly) bunk :-)

@RoyTinker
Copy link

@jstedfast Is the MimeMessage a private member variable? Perhaps it could be made protected, then it wouldn't need to be passed to ProcessRcptToResponse().

@NathanLBCooper I suggest overriding ProcessRcptToResponse(), and keeping track of how many addresses have failed there (via the proposed MimeMessage argument or otherwise). If all of them fail, you could throw an exception and be done.

@RoyTinker
Copy link

On second thought, it would be really nice if:

  • ProcessRcptToResponse() returned a boolean (true=good, false=bad) instead of throwing exceptions
  • All addresses were enumerated and sent via RCPT TO, and the bad addresses and their response codes collected
  • A separate, overridable method got the list of bad addresses and response codes (and perhaps the MimeMessage too, or make it an option to access as protected) and threw the appropriate exception from there

That may not be possible, just my $0.02 based on what I know.

@jstedfast
Copy link
Owner

The MimeMessage is not actually a member variable of the SmtpClient class, so I can't just make it protected.

The MimeMessage is simply passed around internally when sending (I don't like to have member variables that are only transient unless I need it in a custom subclass of some 3rd party class and I need to work around some API limitation).

Thinking about this more, I think more than just the MimeMessage will need to be passed to ProcessRcptToResponse because there may be duplicate email addresses in the To/Cc/Bcc headers and so it'd be difficult to tell if more RCPT TO responses were expected.

@jstedfast
Copy link
Owner

FWIW, while technically neither of you guys probably need the MimeMessage, since the MimeMessage is the primary source of context for the whole Send operation that is known (by consumers of the SmtpClient API at least), I think it's a good piece of data to pass in.

Also I think it might be something developers could find useful in the sense that if, for example, a RCPT TO command fails, they could update the MimeMessage object by removing occurrences of that address in the headers. May be useful, may be not, I don't know... but it seems like the sort of info that a good API might provide.

@RoyTinker
Copy link

Agreed, that's a useful piece of data.

What do you think about collecting errored RCPT TO addresses and throwing an exception at the end (instead of throwing immediately after the first error)?

jstedfast added a commit that referenced this issue Mar 12, 2016
…TO commands

Fixes issue #309 and improved the solution for issue #256
@jstedfast jstedfast added the enhancement New feature or request label Mar 12, 2016
@jstedfast
Copy link
Owner

After thinking about this for a while, I've found a solution that I think will work for everyone.

I've dropped ProcessRcptToResponse which was kind of a gross API anyway and replaced it with:

OnSenderAccepted, OnSenderNotAccepted, OnRecipientAccepted, OnRecipientNotAccepted, and OnNoRecipientsAccepted.

Here's now I envision @NathanLBCooper could implement what he needs:

public class MySmtpClient : SmtpClient
{
    readonly List<SmtpCommandException> exceptions = new List<SmtpCommandException> ();

    protected override void OnSenderAccepted (MimeMessage message, MailboxAddress mailbox, SmtpResponse response)
    {
        exceptions.Clear ();
    }

    protected override void OnRecipientNotAccepted (MimeMessage message, MailboxAddress mailbox, SmtpResponse response)
    {
        try {
            base.OnRecipientNotAccepted (message, mailbox, response);
        } catch (SmtpCommandException ex) {
            exceptions.Add (ex);
        }
    }

    protected override void OnNoRecipientsAccepted (MimeMessage message)
    {
        if (exceptions.Count == 1)
            throw exceptions[0];

        throw new AggregateException (exceptions.ToArray ());
    }
}

@jstedfast
Copy link
Owner

FWIW, these changes are included in the MailKit 1.2.21 release that I just published to nuget.org

@RoyTinker
Copy link

Thanks @jstedfast.

@RoyTinker
Copy link

@jstedfast, this is really nice. Previously, I had to copy/paste the ProcessRcptToResponse() method body into my override, then modify as needed -- a messy job indeed. This is much cleaner.

@jstedfast
Copy link
Owner

Excellent :-)

@NathanLBCooper
Copy link
Author

Thanks @jstedfast. As Tim says, this is a really clean solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question A question about how to do something
Projects
None yet
Development

No branches or pull requests

3 participants