close
The Wayback Machine - https://web.archive.org/web/20201114121951/https://github.com/letsencrypt/boulder/pull/4042
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

Fix mailer error bug #4042

Merged
merged 2 commits into from Feb 8, 2019
Merged

Fix mailer error bug #4042

merged 2 commits into from Feb 8, 2019

Conversation

@rolandshoemaker
Copy link
Contributor

@rolandshoemaker rolandshoemaker commented Feb 7, 2019

This fixes two bugs:

  1. resetAndError would be called on every error, including io.EOF, which is returned when the connection is terminated. Calling m.client.Reset() after a io.EOF will result in another error, causing us to wrap the io.EOF with a errors.errorString. This broke a check in sendMail that was used to cause a reconnect.
  2. There was a error type cast that assumed the type without checking it, which could result in a panic when an error of the unexpected type was returned.
@rolandshoemaker rolandshoemaker requested a review from letsencrypt/boulder-developers as a code owner Feb 7, 2019
Copy link
Contributor

@jsha jsha left a comment

Thanks for working on this! Can you update the PR description to say what the problem was and how you're fixing it?

// return from SendMail() with the error
m.stats.Inc("SendMail.Errors", 1)
return err
if protoErr, ok := err.(*textproto.Error); ok {

This comment has been minimized.

@jsha

jsha Feb 7, 2019
Contributor

I think you can hoist this type-check to where we currently have if err != nil. So we'd have:

if err == nil {
} else if err == io.EOF {
} else if protoErr, ok := err.(*textproto.Error); ok && protoErr == 421 {
} else if protoErr, ok := err.(*textproto.Error); ok && recoverableErrorCodes[protoErr.Code] {
} else {
 ... return err ...
}

Also note this just uses the bool value in recoverableErrorCodes instead of using the , ok method since it's a map of bools.

@jsha
Copy link
Contributor

@jsha jsha commented Feb 7, 2019

Oh also Travis is failing. :-)

@jsha
jsha approved these changes Feb 8, 2019
Copy link
Contributor

@jsha jsha left a comment

Nice work!

@rolandshoemaker rolandshoemaker merged commit 5d6196e into master Feb 8, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.