fixing redial issue, in case of error we need to dial again

arundudeemmt commented 5 years ago

this is fix for issue https://github.com/go-gomail/gomail/issues/121

arundudeemmt commented 5 years ago

@alexcesaro please review and merge

arundudeemmt commented 5 years ago

@alexcesaro not sure why its failing build https://travis-ci.org/go-gomail/gomail/builds/414841472?utm_source=github_status&utm_medium=notification .please help

pedromorgan commented 5 years ago

see #108

ivy commented 5 years ago

@arundudeemmt This might actually be fixed in go-mail/mail@45ab546e07939a7cdd144f97a05ddcec2179dc41. Could you give go-mail a try and let us know if you still see this bug?

arundudeemmt commented 5 years ago

@ivy i tried go-mail .it also has same issue as with go-gomail .After certain time interval , get write pipe broken error .

arundudeemmt commented 5 years ago

@pedromorgan @ivy please help us in closing this quickly.. we are about to roll out QOR in production and using this go-mail module ... this has kind of bottleneck ... trying to close it from quite a while ..... i am hoping if this fix gets in library then use updated library else will have to our forked version with custom fix in it which i dont wish to do

ivy commented 5 years ago

@cryptix: I was looking through QOR and noticed you submitted qor/mailer#1 to solve a similar issue. Is that at all related to this issue?

From looking at the repo, it also looks like they might not have switched to go-mail like they intended in https://github.com/qor/mailer/commit/0555e49f99acf5467a77747fc855d7e005d902c1#diff-04c6e90faac2675aa89e2176d2eec7d8R17

@arundudeemmt I don't want to stop you from going to production so definitely don't hesitate to use your own fork in the meantime. I do have a responsibility though to ensure every bug fix has a minimal impact on the other users of this library. I just have a couple questions still:

Are you absolutely certain you've switched over to go-mail/mail and aren't still using go-gomail? You can verify this by checking for github.com/go-mail/mail in the output of this command (run it in the same directory as the package you're using): go list -f '{{ join .Deps "\n" }}'

If you can confirm that you are using go-mail/mail could you also try applying this patch to your copy? This way, we can see the exact error and its type (I think you're getting syscall.EPIPE but I do want to double-check):

diff --git a/smtp.go b/smtp.go
index 547e04d..e377eea 100644
--- a/smtp.go
+++ b/smtp.go
@@ -4,6 +4,7 @@ import (
    "crypto/tls"
    "fmt"
    "io"
+    "log"
    "net"
    "net/smtp"
    "strings"
@@ -234,6 +235,7 @@ func (c *smtpSender) Send(from string, to []string, msg io.WriterTo) error {
    }

    if err := c.Mail(from); err != nil {
+        log.Printf("mail: smtpSender.Send error: %s (%T)", err, err)
        if c.retryError(err) {
            // This is probably due to a timeout, so reconnect and try again.
            sc, derr := c.d.Dial()

Thank you for being patient. We should be able to finally resolve this once we have all the details. 😄

cryptix commented 5 years ago

I was looking through QOR and noticed you submitted qor/mailer#1 to solve a similar issue. Is that at all related to this issue?

@ivy: yup, spot on. It is also the reason I opened https://github.com/go-mail/mail/issues/15. Thanks for digging up the import weirdness! I hope @jinzhu finds some time to address this...

jagritisinghal commented 5 years ago

@ivy - This is in continuation to the problem faced by @arundudeemmt . Regarding the two points you mentioned, I am getting github.com/go-mail/mail after running the command - go list -f '{{ join .Deps "\n" }}' Also, the error that I got was - *mail: smtpSender.Send error: 451 4.4.2 Timeout - closing connection. p26-v6sm11574453pfi.183 - gsmtp (textproto.Error)**

Please help in resolving the issue. Thanks!

ivy commented 5 years ago

@jagritisinghal That error's coming directly from the SMTP server you're connected to. I imagine your connection's sitting idle for some time until Gmail decides to close it, right? Unfortunately, this library can only attempt to redial when the client-side times out. Otherwise, we end up dealing with lots of edge cases. SMTP services are wildly different and all have their own errors. See https://github.com/go-mail/mail/issues/15#issuecomment-414166154 for the discussion we're having around that.

I'd recommend making your fix in qor/mailer (or wherever you're interacting with go-mail) in the meantime and construct a new Sender instead. It isn't the prettiest solution but it's your best bet until I or someone else comes up with a solution that works for everyone.

arundudeemmt commented 5 years ago

@ivy if you dont mind , can you please explain is there any downside of redial if there is any kind of error ? Thats ways we can tackle many error cases and make this mail module more reliable IMO .

theckman commented 5 years ago

@arundudeemmt one of the Go Proverbs (a Go idiom) is this:

Don't just check errors, handle them gracefully.

But what does this mean? It means when looking to see if an error occurred, it should not be a binary evaluation. Instead you should inspect the error, and understand how you can handle that specific error case.

In this patch, we aren't handling the error gracefully. We are only checking it, and taking an action no matter the result. I don't feel this is a safe default, nor is it idiomatic Go.

ivy commented 5 years ago

@arundudeemmt The biggest downside of simply redialing is that you could find yourself being blacklisted by email services. For example, if one day your credentials changed, your application would constantly reconnect and be immediately disconnected. After too many failures, most services will simply ban your IP address for a few hours and depending on how long it takes you to resolve things on your end, that ban could extend to a few days.

If I were to lay things out, I'd say:

1. Log your errors

The most important thing you should be doing when recovering from a delivery error is to log it somewhere where you'll be able to see it. An error notifier like Sentry, some log aggregation service, or even something as simple as a Slack channel would work fine, just be sure that you're keeping track of it so you're never surprised. That alone will save you from the worst case scenario.

2. Implement a reconnect backoff

The next best thing is to add a reconnect backoff. Good clients wait a short time before redialing. It prevents my earlier example from getting worse because you'll be increasing that retry delay each time a connect fails.

3. Dial & close or use a connection pool

Lastly, go-mail's SMTP sender is intended to be closed when you no longer need it. If you're planning to have one-off deliveries as part of an HTTP transaction, you should probably close the connection when you're done with it.

Otherwise, if you're processing large batches in the background somewhere, it's a good idea to have a wrapper responsible for maintaining the connection (possibly multiple connections) and redialing in the event of an error. Connection pooling is a bit too much to explain here but that's one feature that go-mail could seriously benefit from.

I hope that answers your questions. If you need any more assistance, let me know. 😄

go-gomail / gomail