On Sun, Oct 20, 2019 at 2:16 AM Yasuhito FUTATSUKI <[email protected]>
wrote:
> On 2019/10/20 13:46, Nathan Hartman wrote:
> > From the "Closing Old Issues" department:
> >
> > SVN-1804: "mailer.py doesn't catch SMTPRecipientsRefused in finish()"
> > * Created and last updated in 2004.
> > * No comments in more than 15 years.
> >
> > From my reading of this issue, the impact is that any mail delivery
> > hiccup may cause this hook script to exit with an unhandled exception,
> > impeding any further mails that it might have sent.
>
(snip)
> > Option 1: Handle (and ignore) exceptions in SMTPOutput.finish() to let
> > script execution continue after any SMTP failure. This could be as
> > simple as wrapping the contents of SMTPOutput.finish() in a "try:" ..
> > "except:" (catch-all), where the except block does nothing.
>
> With this approach, I think the script should exit with code
> other than 0 if an exception raised, to notify error occured to hook
> scripts. Python script exits with code 1 if the script is termitated
> by unhandled exceptions, so it can notify on current implementation.
> Hook scripts can also watch stderr to log what happens on mailer.py,
> etc., though our post-*.tmpl don't do it.
I think the attached patch accomplishes that. (I'm not putting it inline
because it's 200 lines long! Most of them due to indent changes.)
I'd appreciate a review!
The changes are pretty basic:
* A new exception class is added: MessageSendFailure.
* The contents of SMTPOutput.finish() are wrapped in try..except. We
catch any smtplib.SMTPException, print an error to stderr, and raise
MessageSendFailure.
* In all classes' generate(), we wrap the body of the for-loop in a
try..except. We catch MessageSendFailure and increment a counter;
generate() returns the value of the counter.
* In main, the return value of messenger.generate() is returned to
main's caller.
* In the __main__ program, the call to svn.core.run_app() is wrapped
with sys.exit().
Index: tools/hook-scripts/mailer/mailer.py
===================================================================
--- tools/hook-scripts/mailer/mailer.py (revision 1868807)
+++ 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,15 +285,19 @@
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()
+ 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)
+ 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()
+ except smtplib.SMTPException as detail:
+ sys.stderr.write("SMTP error occurred: %s" % detail);
+ raise MessageSendFailure;
class StandardOutput(OutputBase):
@@ -421,21 +425,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 +465,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 +578,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 +1413,8 @@
pass
class UnknownSubcommand(Exception):
pass
+class MessageSendFailure(Exception):
+ pass
if __name__ == '__main__':
@@ -1455,8 +1476,8 @@
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])
+ sys.exit(svn.core.run_app(main, cmd, config_fname, repos_dir,
+ sys.argv[3:3+expected_args]))
# ------------------------------------------------------------------------
# TODO
