make sure to set timeout before read from the connection

[aaabbbccc-123]

No description provided.

[steviesama]

I wonder if this could cause DialAndSend() to not return. I've had it sit there for 20 minutes before...it's happened 4 times in a week of mass emails. I stop the server and restart to reload the email campaign jobs...and they all send like they should.

This is the current order those functions are in currently in my copy...I'm thinking the smtpNewClient() is where it could hang without the deadline possibly?

  if d.SSL {
    conn = tlsClient(conn, d.tlsConfig())
  }

  c, err := smtpNewClient(conn, d.Host)
  if err != nil {
    return nil, err
  }

  if d.Timeout > 0 {
    conn.SetDeadline(time.Now().Add(d.Timeout))
  }

[aaabbbccc-123]

ya, the current code can have the connection hanging forever without a deadline. the tls handshake does send/receive some network traffic.

[steviesama]

@aaabbbccc-123 Thanks...this is likely my problem then...I'm waiting for at least one more hang occurrence before I shift the placement of the SetDeadline() call...I took all the time to place the logging so I wanna see precisely where it's stopping.

When it hangs again...hopefully shifting that deadline up will fix it.

[steviesama]

@aaabbbccc-123 Question...in the event it hangs due to this deadline not being called...is the error that it should have generated if it timeout'd out correctly the gomail error that ends i/o timeout? Cause I did actually have that error successfully generated a couple of times...but I usually never see that error...it usually just hangs.

[steviesama]

I've finally experienced another hang in relation to what I think this pull request fixes. I've got a log of where the code hangs in the repo call. The Log excerpt for the send cycle it hung on is below and the bottom call was the last time it responded. I have since moved the conn.SetDeadline() call to be above any reads from the connection. I'll check back in after a week or so, so that the production code has had time for the emailer to be used frequently enough to have had time to hang under the previous conditions...actually I'll wait 2 weeks...It usually hangs once or twice a week though.

===> BEFORE CALL sendMail('76036 (2014 / Freightliner / Cascadia) Memphis, Tennessee')
===> START CALL sendMail('holsttrk@atcnet.net')
==> sendMail(): m := gomail.NewMessage()
==> sendMail(): m.SetHeader("From", from.Username)
==> sendMail(): m.SetHeader("To", to)
==> sendMail(): m.SetHeader("Subject", subj)
==> BEFORE sendMail(): attachments
==> AFTER sendMail(): attachments
==> sendMail(): m.SetBody("text/html", body)
==> sendMail(): d := gomail.NewDialer(
==> sendMail(): d.StartTLSPolicy = gomail.MandatoryStartTLS
==> sendMail(): err := d.DialAndSend(m)
===> DialAndSend(): s, err := d.Dial()
===> Dial(): conn, err := NetDialTimeout("tcp", addr(d.Host, d.Port), d.Timeout)
===> Dial(): if d.SSL {
===> Dial(): c, err := smtpNewClient(conn, d.Host)

[steviesama]

This pull request fixes my problem in #56. I've implemented it locally if someone can merge this.

[fredbi]

Also tested this with local fork and this simple trick is efficient! Please merge

[ivy]

Hey folks - Apologies, I haven't had much time to give this PR attention.

If I follow correctly, this is a bug brought on by smtp.NewClient attempting to read from the connection before we've set a deadline. That definitely seems like a cause for a deadlock.

I'll get a test written and have this merged in over the weekend.

[bgehman]

@ivy Hello, we are seeing similar hang issues -- mind getting this fix merged?

[afk11]

Hey, this looks like it solves my issue, and I can see new issues about this as well. Can we merge this please? Also might be worth backporting to older versions people might still be using

[phanirithvij]

@ivy I apologize for pinging, can this be merged?

[liuliqiang]

I meet the same problem again. So it merge this pr to my fork project: https://github.com/goself/mail，I will keep maintaining this project。

I help this would help someone.