wneessen / go-mail

📧 Easy to use, yet comprehensive library for sending mails with Go
https://go-mail.dev
MIT License
571 stars 44 forks source link

Provide a way of knowing which emails failed/succeeded in sending #90

Closed imirkin closed 1 year ago

imirkin commented 1 year ago

Is your feature request related to a problem? Please describe.

When sending multiple emails, an error is returned when any one of them fails. How do I know which one failed? I have, let's say, 100 emails to send. I get an error back ... now what? Which one(s) do I try resending? Which ones do I mark internally as having been sent / failed to send?

Describe the solution you'd like

Perhaps it makes sense to return an array of errors, 1:1 with the messages, guaranteed to be the same length as the messages? That makes it slightly annoying for the checkConn-type global failure. OTOH, this not exactly the common case, so OK to just make N copies of that error in an array? Not exactly idiomatic Go, but I don't know what is in this case.

Describe alternatives you've considered

Perhaps something can be added to the Msg itself indicating send status? Seems dodgy to mutate inputs though, and what happens if the object is reused somehow (e.g. resent), could make it unclear whether the status is "correct" or not.

Additional context

Note that I'm not using this library yet. Just shopping around for a lib that can send multiple emails in a single connection.

imirkin commented 1 year ago

Hm, answering myself, perhaps the solution is to just Send() with one Msg per invocation, reusing the same client. But then why does Send() take multiple args?

wneessen commented 1 year ago

Thanks for the issue @imirkin. That's actually an interesting problem. I assume most sending mail servers would actually provide an error message that holds i. e. the recipients mail address, but that's probably not the default for everything. I'll think about it and come up with a possible solution and open it for discussion.

imirkin commented 1 year ago

I don't think that mail servers return errors in any standardized format. A human reading the error could maybe work it out, but even that's not guaranteed. Note that the emails may be going to the same recipients, etc.

wneessen commented 1 year ago

I am proposing this change: https://github.com/wneessen/go-mail/compare/main...feature/90_provide-a-way-of-knowing-which-emails-failedsucceeded-in-sending

It introduces the SendError type which implements the error interface. A new senderror field has been added to the Msg as well, to introduce this type to it.

I've also added different error variables that indicate the different things that can go wrong during mail delivery. These variables can be checked for, for each Msg using the errors.As method

The Error() method of SendError will return a detailed error string on why the Msg could not be delivered.

Additionally, HasSendError() and SendError() methods have been added to Msg. While HasSendError() simply returns a bool in case a Msg failed during delivery, the SendError() will return the full SendError error type.

I think it's also fair that we reset the Msg.senderror field to nil before we Client.Send() it. Since the send error indicate an error during the mail delivery, in my opinion it should be reset when a re-send is initiated, so that the Msg.senderror field always represents the latest delivery error. The send error should be checked after mail delivery and before a retry is started.

wneessen commented 1 year ago

While working on a test for the different test types, I noticed and implementation issue, so the commits made, are not final yet.

imirkin commented 1 year ago

I think there's probably enough info, but it's also important to be able to distinguish between a "retriable" error, e.g. server just plain disconnects (let's say it got restarted), and a permanent error, e.g. "cannot deliver to recipient", etc. I believe the error codes from SMTP commands indicate the kind of error that it is.

wneessen commented 1 year ago

True. That would involve some SMTP response message parsing. Let me check what can be done there

wneessen commented 1 year ago

I'ver overhauled the commit and fixed the error handling. We've also now a isTemp field in the error, that indicates if it's a retryable error or not.

iwittkau commented 1 year ago

Took a look at the code because I'm interested in the isTemp flag.

I think it would make sense to add this method to the error type as well because error values would be checked first.

I'd want to use it like this:

if err := client.DialAndSendWithContext(ctx, msg); err != nil {
    var sendErr mail.SendError
    if errors.As(err, &sendErr) && sendErr.IsTemp() {
        // retryable
        return nil, fmt.Errorf("%w: %s", ErrTemporary, sendErr.Error())
    }
    // probably non-retryable
    return nil, fmt.Errorf("%w: %s", ErrSending, err.Error())
}

Also consider removing the client_send string prefix from the error messages, because this can look strange or redundant when library users wrap errors.

wneessen commented 1 year ago

Good point indeed @iwittkau. I've adapted the proposal and changed the Send method to return the SendError instead of a fmt.Error, so we can use the code you proposed for checking for temporary errors after delivery.

wneessen commented 1 year ago

I'm pretty happy with the proposal and created a corresponding PR. If no further comments or suggestions are added to this discussion, I'll merge it tomorrow and release v0.3.7.