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

[arundudeemmt]

this is fix for issue #121

[arundudeemmt]

@alexcesaro please review and merge

[arundudeemmt]

@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]

see #108

[ivy]

There's an important distinction in error handling here: Gomail shouldn't make any attempts to redial unless it was explicitly an EOF. Any failure should instead be handled by the caller.

@arundudeemmt You mentioned in #104 that QOR relies on Gomail. Could you link me to where in the codebase it's used? I'd recommend extending that project to account for errors as it isn't always the best choice to simply redial without any additional work.

[arundudeemmt]

some time there are error which are not EOF too and requires a redial like write: broken pipe .For broken pipe i was not able to find any standard error codes from smtp .Please suggest a way to fix the same .
IMO broken pipe comes when there is no event in last x time frame and pipe is broken .So three ways to fix i see it as 1) redial 2)build a keep alive mechanism and keep pinging server after x interval where x is less than time out interval 3)close connection after every email and then redial

[arundudeemmt]

@ivy @pedromorgan did you got a chance to look at this ?

[ivy]

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

[arundudeemmt]

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

[arundudeemmt]

@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]

@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 qor/mailer@0555e49#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]

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 go-mail#15.
Thanks for digging up the import weirdness! I hope @jinzhu finds some time to address this...

[jagritisinghal]

@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]

@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 go-mail#15 (comment) 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]

@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]

@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]

@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. 😄