Thanks to everyone for reviewing and providing feedback!
I've incorporated it and I hope that the following is good enough to
be committed as a fix for SVN-1804...
Since last time, only SMTPOutput.finish changes. I think it will be
easier to just see the function in full rather than a diff...
This is the original function before I began:
[[[
def finish(self):
if self.cfg.is_set('general.smtp_ssl') and self.cfg.general.smtp_ssl ==
'yes':
server = smtplib.SMTP_SSL(self.cfg.general.smtp_hostname)
else:
server = smtplib.SMTP(self.cfg.general.smtp_hostname)
if self.cfg.is_set('general.smtp_username'):
server.login(self.cfg.general.smtp_username,
self.cfg.general.smtp_password)
server.sendmail(self.from_addr, self.to_addrs, self.buffer.getvalue())
server.quit()
]]]
After incorporating all the preceding feedback, the function is
considerably longer, but we handle and report errors:
[[[
def finish(self):
"""
Send email via SMTP or SMTP_SSL, logging in if username is
specified.
Errors such as invalid recipient, which affect a particular email,
are reported to stderr and raise MessageSendFailure. If the caller
has other emails to send, it can continue doing so.
Errors caused by bad configuration, such as login failures, for
which multiple retries could lead to SMTP server lockout, are
reported to stderr and re-raised. These should be considered fatal.
"""
try:
if self.cfg.is_set('general.smtp_ssl') and self.cfg.general.smtp_ssl
== 'yes':
server = smtplib.SMTP_SSL(self.cfg.general.smtp_hostname)
else:
server = smtplib.SMTP(self.cfg.general.smtp_hostname)
except Exception as detail:
sys.stderr.write("mailer.py: Failed to instantiate SMTP object: %s\n"
% (detail,))
# Any error to instantiate is fatal
raise
try:
if self.cfg.is_set('general.smtp_username'):
try:
server.login(self.cfg.general.smtp_username,
self.cfg.general.smtp_password)
except smtplib.SMTPException as detail:
sys.stderr.write("mailer.py: SMTP login failed with username %s
and/or password: %s\n"
% (self.cfg.general.smtp_username, detail,))
# Any error at login is fatal
raise
server.sendmail(self.from_addr, self.to_addrs, self.buffer.getvalue())
except smtplib.SMTPRecipientsRefused as detail:
sys.stderr.write("mailer.py: SMTP recipient(s) refused: %s: %s\n"
% (self.to_addrs, detail,))
raise MessageSendFailure
except smtplib.SMTPSenderRefused as detail:
sys.stderr.write("mailer.py: SMTP sender refused: %s: %s\n"
% (self.from_addr, detail,))
raise MessageSendFailure
except smtplib.SMTPException as detail:
# All other errors are fatal; this includes:
# SMTPHeloError, SMTPDataError, SMTPNotSupportedError
sys.stderr.write("mailer.py: SMTP error occurred: %s\n" % (detail,))
raise
finally:
server.quit()
]]]
Proposed log message:
[[[
Fix issue #1804: mailer.py terminates abnormally on any SMTP error
Per SVN-1804, any SMTP error terminates mailer.py with an unhandled
exception. The impact is that emails which would have been sent after
the exception are silently lost.
To fix this issue, we handle SMTP exceptions. When an exception only
affects a particular email, such as invalid recipient, execution
continues to avoid losing any later emails. Fatal SMTP errors, such
as login with bad credentials, terminate execution as before but with
some additional reporting to stderr.
The script's exit code is zero if all messages sent successfully,
nonzero if any SMTP error occurs.
* tools/hook-scripts/mailer/mailer.py
(main): Propagate return value of messenger.generate() to caller.
(MessageSendFailure): New exception class, to allow for future
handling of such failures with other (non-SMTP) delivery methods.
(SMTPOutput.finish): Reimplement with exception handling and
reporting to stderr.
(Commit.generate, PropChange.generate, Lock.generate): Wrap contents
of for-loop in try .. except block; return nonzero if SMTP
error(s) occurred.
(__main__): Exit nonzero if SMTP error(s) occurred.
Found by: Robert M. Zigweid
Review by: danielsh (partial review)
futatuki
]]]
Secondary diff, just for fun. More below...
[[[
Index: tools/hook-scripts/mailer/mailer.py
===================================================================
--- tools/hook-scripts/mailer/mailer.py (revision 1869113)
+++ tools/hook-scripts/mailer/mailer.py (working copy)
@@ -123,7 +123,7 @@ def main(pool, cmd, config_fname, repos_dir, cmd_a
else:
raise UnknownSubcommand(cmd)
- messenger.generate()
+ return messenger.generate()
def remove_leading_slashes(path):
@@ -285,14 +285,59 @@ class SMTPOutput(MailedOutput):
self.write(self.mail_headers(group, params))
def finish(self):
+ """
+ Send email via SMTP or SMTP_SSL, logging in if username is
+ specified.
+
+ Errors such as invalid recipient, which affect a particular email,
+ are reported to stderr and raise MessageSendFailure. If the caller
+ has other emails to send, it can continue doing so.
+
+ Errors caused by bad configuration, such as login failures, for
+ which multiple retries could lead to SMTP server lockout, are
+ reported to stderr and re-raised. These should be considered fatal.
+ """
+
+ try:
if self.cfg.is_set('general.smtp_ssl') and self.cfg.general.smtp_ssl
== 'yes':
server = smtplib.SMTP_SSL(self.cfg.general.smtp_hostname)
else:
server = smtplib.SMTP(self.cfg.general.smtp_hostname)
+ except Exception as detail:
+ sys.stderr.write("mailer.py: Failed to instantiate SMTP object:
%s\n" % (detail,))
+ # Any error to instantiate is fatal
+ raise
+
+ try:
if self.cfg.is_set('general.smtp_username'):
+ try:
server.login(self.cfg.general.smtp_username,
self.cfg.general.smtp_password)
+ except smtplib.SMTPException as detail:
+ sys.stderr.write("mailer.py: SMTP login failed with username %s
and/or password: %s\n"
+ % (self.cfg.general.smtp_username, detail,))
+ # Any error at login is fatal
+ raise
+
server.sendmail(self.from_addr, self.to_addrs, self.buffer.getvalue())
+
+ except smtplib.SMTPRecipientsRefused as detail:
+ sys.stderr.write("mailer.py: SMTP recipient(s) refused: %s: %s\n"
+ % (self.to_addrs, detail,))
+ raise MessageSendFailure
+
+ except smtplib.SMTPSenderRefused as detail:
+ sys.stderr.write("mailer.py: SMTP sender refused: %s: %s\n"
+ % (self.from_addr, detail,))
+ raise MessageSendFailure
+
+ except smtplib.SMTPException as detail:
+ # All other errors are fatal; this includes:
+ # SMTPHeloError, SMTPDataError, SMTPNotSupportedError
+ sys.stderr.write("mailer.py: SMTP error occurred: %s\n" % (detail,))
+ raise
+
+ finally:
server.quit()
@@ -421,11 +466,13 @@ class Commit(Messenger):
### rather than rebuilding it each time.
subpool = svn.core.svn_pool_create(self.pool)
+ ret = 0
# build a renderer, tied to our output stream
renderer = TextCommitRenderer(self.output)
for (group, param_tuple), (params, paths) in self.groups.items():
+ try:
self.output.start(group, params)
# generate the content for this group and set of params
@@ -433,9 +480,12 @@ class Commit(Messenger):
group, params, paths, subpool)
self.output.finish()
+ except MessageSendFailure:
+ ret = 1
svn.core.svn_pool_clear(subpool)
svn.core.svn_pool_destroy(subpool)
+ return ret
class PropChange(Messenger):
@@ -456,7 +506,9 @@ class PropChange(Messenger):
def generate(self):
actions = { 'A': 'added', 'M': 'modified', 'D': 'deleted' }
+ ret = 0
for (group, param_tuple), params in self.groups.items():
+ try:
self.output.start(group, params)
self.output.write('Author: %s\n'
'Revision: %s\n'
@@ -485,6 +537,9 @@ class PropChange(Messenger):
'to' : tempfile2.name,
}))
self.output.finish()
+ except MessageSendFailure:
+ ret = 1
+ return ret
def get_commondir(dirlist):
@@ -564,7 +619,9 @@ class Lock(Messenger):
self.dirlist[0], self.pool)
def generate(self):
+ ret = 0
for (group, param_tuple), (params, paths) in self.groups.items():
+ try:
self.output.start(group, params)
self.output.write('Author: %s\n'
@@ -579,6 +636,9 @@ class Lock(Messenger):
self.output.write('Comment:\n%s\n' % (self.lock.comment or ''))
self.output.finish()
+ except MessageSendFailure:
+ ret = 1
+ return ret
class DiffSelections:
@@ -1394,6 +1454,8 @@ class UnknownMappingSpec(Exception):
pass
class UnknownSubcommand(Exception):
pass
+class MessageSendFailure(Exception):
+ pass
if __name__ == '__main__':
@@ -1455,8 +1517,9 @@ if the property was added, modified or deleted, re
if not os.path.exists(config_fname):
raise MissingConfig(config_fname)
- svn.core.run_app(main, cmd, config_fname, repos_dir,
+ ret = svn.core.run_app(main, cmd, config_fname, repos_dir,
sys.argv[3:3+expected_args])
+ sys.exit(1 if ret else 0)
# ------------------------------------------------------------------------
# TODO
]]]
Daniel, thank you for finding rzigweid's real name. I didn't know how
to do that. :-) I agree that the glob pattern in the log message is a
bad idea as it reduces searchability. I've changed this to list each
affected function explicitly.
Yasuhito, thank you for reviewing. I hope the code above addresses
all the very good points you made.
Please let me know if this looks acceptable to be committed...
Thanks again to everyone!
Nathan
Index: tools/hook-scripts/mailer/mailer.py
===================================================================
--- tools/hook-scripts/mailer/mailer.py (revision 1869113)
+++ tools/hook-scripts/mailer/mailer.py (working copy)
@@ -123,7 +123,7 @@
else:
raise UnknownSubcommand(cmd)
- messenger.generate()
+ return messenger.generate()
def remove_leading_slashes(path):
@@ -285,17 +285,62 @@
self.write(self.mail_headers(group, params))
def finish(self):
- if self.cfg.is_set('general.smtp_ssl') and self.cfg.general.smtp_ssl ==
'yes':
- server = smtplib.SMTP_SSL(self.cfg.general.smtp_hostname)
- else:
- server = smtplib.SMTP(self.cfg.general.smtp_hostname)
- if self.cfg.is_set('general.smtp_username'):
- server.login(self.cfg.general.smtp_username,
- self.cfg.general.smtp_password)
- server.sendmail(self.from_addr, self.to_addrs, self.buffer.getvalue())
- server.quit()
+ """
+ Send email via SMTP or SMTP_SSL, logging in if username is
+ specified.
+ Errors such as invalid recipient, which affect a particular email,
+ are reported to stderr and raise MessageSendFailure. If the caller
+ has other emails to send, it can continue doing so.
+ Errors caused by bad configuration, such as login failures, for
+ which multiple retries could lead to SMTP server lockout, are
+ reported to stderr and re-raised. These should be considered fatal.
+ """
+
+ try:
+ if self.cfg.is_set('general.smtp_ssl') and self.cfg.general.smtp_ssl ==
'yes':
+ server = smtplib.SMTP_SSL(self.cfg.general.smtp_hostname)
+ else:
+ server = smtplib.SMTP(self.cfg.general.smtp_hostname)
+ except Exception as detail:
+ sys.stderr.write("mailer.py: Failed to instantiate SMTP object: %s\n" %
(detail,))
+ # Any error to instantiate is fatal
+ raise
+
+ try:
+ if self.cfg.is_set('general.smtp_username'):
+ try:
+ server.login(self.cfg.general.smtp_username,
+ self.cfg.general.smtp_password)
+ except smtplib.SMTPException as detail:
+ sys.stderr.write("mailer.py: SMTP login failed with username %s
and/or password: %s\n"
+ % (self.cfg.general.smtp_username, detail,))
+ # Any error at login is fatal
+ raise
+
+ server.sendmail(self.from_addr, self.to_addrs, self.buffer.getvalue())
+
+ except smtplib.SMTPRecipientsRefused as detail:
+ sys.stderr.write("mailer.py: SMTP recipient(s) refused: %s: %s\n"
+ % (self.to_addrs, detail,))
+ raise MessageSendFailure
+
+ except smtplib.SMTPSenderRefused as detail:
+ sys.stderr.write("mailer.py: SMTP sender refused: %s: %s\n"
+ % (self.from_addr, detail,))
+ raise MessageSendFailure
+
+ except smtplib.SMTPException as detail:
+ # All other errors are fatal; this includes:
+ # SMTPHeloError, SMTPDataError, SMTPNotSupportedError
+ sys.stderr.write("mailer.py: SMTP error occurred: %s\n" % (detail,))
+ raise
+
+ finally:
+ server.quit()
+
+
class StandardOutput(OutputBase):
"Print the commit message to stdout."
@@ -421,21 +466,26 @@
### rather than rebuilding it each time.
subpool = svn.core.svn_pool_create(self.pool)
+ ret = 0
# build a renderer, tied to our output stream
renderer = TextCommitRenderer(self.output)
for (group, param_tuple), (params, paths) in self.groups.items():
- self.output.start(group, params)
+ try:
+ self.output.start(group, params)
- # generate the content for this group and set of params
- generate_content(renderer, self.cfg, self.repos, self.changelist,
- group, params, paths, subpool)
+ # generate the content for this group and set of params
+ generate_content(renderer, self.cfg, self.repos, self.changelist,
+ group, params, paths, subpool)
- self.output.finish()
+ self.output.finish()
+ except MessageSendFailure:
+ ret = 1
svn.core.svn_pool_clear(subpool)
svn.core.svn_pool_destroy(subpool)
+ return ret
class PropChange(Messenger):
@@ -456,35 +506,40 @@
def generate(self):
actions = { 'A': 'added', 'M': 'modified', 'D': 'deleted' }
+ ret = 0
for (group, param_tuple), params in self.groups.items():
- self.output.start(group, params)
- self.output.write('Author: %s\n'
- 'Revision: %s\n'
- 'Property Name: %s\n'
- 'Action: %s\n'
- '\n'
- % (self.author, self.repos.rev, self.propname,
- actions.get(self.action, 'Unknown (\'%s\')' \
- % self.action)))
- if self.action == 'A' or self.action not in actions:
- self.output.write('Property value:\n')
- propvalue = self.repos.get_rev_prop(self.propname)
- self.output.write(propvalue)
- elif self.action == 'M':
- self.output.write('Property diff:\n')
- tempfile1 = tempfile.NamedTemporaryFile()
- tempfile1.write(sys.stdin.read())
- tempfile1.flush()
- tempfile2 = tempfile.NamedTemporaryFile()
- tempfile2.write(self.repos.get_rev_prop(self.propname))
- tempfile2.flush()
- self.output.run(self.cfg.get_diff_cmd(group, {
- 'label_from' : 'old property value',
- 'label_to' : 'new property value',
- 'from' : tempfile1.name,
- 'to' : tempfile2.name,
- }))
- self.output.finish()
+ try:
+ self.output.start(group, params)
+ self.output.write('Author: %s\n'
+ 'Revision: %s\n'
+ 'Property Name: %s\n'
+ 'Action: %s\n'
+ '\n'
+ % (self.author, self.repos.rev, self.propname,
+ actions.get(self.action, 'Unknown (\'%s\')' \
+ % self.action)))
+ if self.action == 'A' or self.action not in actions:
+ self.output.write('Property value:\n')
+ propvalue = self.repos.get_rev_prop(self.propname)
+ self.output.write(propvalue)
+ elif self.action == 'M':
+ self.output.write('Property diff:\n')
+ tempfile1 = tempfile.NamedTemporaryFile()
+ tempfile1.write(sys.stdin.read())
+ tempfile1.flush()
+ tempfile2 = tempfile.NamedTemporaryFile()
+ tempfile2.write(self.repos.get_rev_prop(self.propname))
+ tempfile2.flush()
+ self.output.run(self.cfg.get_diff_cmd(group, {
+ 'label_from' : 'old property value',
+ 'label_to' : 'new property value',
+ 'from' : tempfile1.name,
+ 'to' : tempfile2.name,
+ }))
+ self.output.finish()
+ except MessageSendFailure:
+ ret = 1
+ return ret
def get_commondir(dirlist):
@@ -564,21 +619,26 @@
self.dirlist[0], self.pool)
def generate(self):
+ ret = 0
for (group, param_tuple), (params, paths) in self.groups.items():
- self.output.start(group, params)
+ try:
+ self.output.start(group, params)
- self.output.write('Author: %s\n'
- '%s paths:\n' %
- (self.author, self.do_lock and 'Locked' or 'Unlocked'))
+ self.output.write('Author: %s\n'
+ '%s paths:\n' %
+ (self.author, self.do_lock and 'Locked' or
'Unlocked'))
- self.dirlist.sort()
- for dir in self.dirlist:
- self.output.write(' %s\n\n' % dir)
+ self.dirlist.sort()
+ for dir in self.dirlist:
+ self.output.write(' %s\n\n' % dir)
- if self.do_lock:
- self.output.write('Comment:\n%s\n' % (self.lock.comment or ''))
+ if self.do_lock:
+ self.output.write('Comment:\n%s\n' % (self.lock.comment or ''))
- self.output.finish()
+ self.output.finish()
+ except MessageSendFailure:
+ ret = 1
+ return ret
class DiffSelections:
@@ -1394,6 +1454,8 @@
pass
class UnknownSubcommand(Exception):
pass
+class MessageSendFailure(Exception):
+ pass
if __name__ == '__main__':
@@ -1455,8 +1517,9 @@
if not os.path.exists(config_fname):
raise MissingConfig(config_fname)
- svn.core.run_app(main, cmd, config_fname, repos_dir,
- sys.argv[3:3+expected_args])
+ ret = svn.core.run_app(main, cmd, config_fname, repos_dir,
+ sys.argv[3:3+expected_args])
+ sys.exit(1 if ret else 0)
# ------------------------------------------------------------------------
# TODO
