You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Nathan Hartman <ha...@gmail.com> on 2019/10/20 04:46:25 UTC
Issue tracker housecleaning: SVN-1804
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.
Looking at mailer.py in trunk@HEAD, SMTPOutput.finish() doesn't handle
any exceptions. In particular:
SMTP.login may raise:
* SMTPHeloError
* SMTPAuthenticationError
* SMTPNotSupportedError
* SMTPException
SMTP.sendmail may raise:
* SMTPRecipientsRefused
* SMTPHeloError
* SMTPSenderRefused
* SMTPDataError
* SMTPNotSupportedError
Any of these exceptions cause the same impact. The same is probably
true of any unhandled exceptions throughout the script, but SMTP errors
are completely outside our control.
So we have a choice to make:
Either:
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.
Option 2: Just close the issue. No comments on this issue in 15 years.
Thoughts?
Re: Issue tracker housecleaning: SVN-1804
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Nathan Hartman wrote on Wed, Oct 23, 2019 at 11:48:28 -0400:
> On Sun, Oct 20, 2019 at 2:16 AM Yasuhito FUTATSUKI <fu...@poem.co.jp>
> wrote:
> > 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.)
In such cases, it's helpful to attach a secondary diff, generated with «svn
diff -x-wp», for review purposes.
> 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().
What is the functional change? A log message should describe the functional
change and the the reasons therefor; it shouldn't simply narrate the diff. Your
last bullet is a good counter-example.
As Yasuhito said, unhandled exceptions already cause a non-zero exit code, so
I'm guessing the point here is that SMTPException during the first delivery no
longer prevent second (and any subsequent) deliveries from being attempted? In
other words, in what case does the patch have a user-visible effect?
> Index: tools/hook-scripts/mailer/mailer.py
> ===================================================================
> --- tools/hook-scripts/mailer/mailer.py (revision 1868639)
[ I am quoting the -x-wp diff, rather than the one Nathan sent. ]
> @@ -294,6 +295,9 @@ class SMTPOutput(MailedOutput):
> 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);
Error messages should be signed:
sys.stderr.write("mailer.py: SMTP error occurred: %s" % (detail,))
# ^^^^^^^^^^^
> @@ -1455,8 +1476,8 @@ 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,
> - sys.argv[3:3+expected_args])
> + sys.exit(svn.core.run_app(main, cmd, config_fname, repos_dir,
> + sys.argv[3:3+expected_args]))
If run_app returns 256, the exit code will be zero. I suggest to do
ret = svn.core.run_app(…)
sys.exit(1 if ret else 0)
I did not review the core functionality changes due to lack of time, sorry.
Cheers,
Daniel
Re: Issue tracker housecleaning: SVN-1804
Posted by Nathan Hartman <ha...@gmail.com>.
On Sun, Oct 20, 2019 at 2:16 AM Yasuhito FUTATSUKI <fu...@poem.co.jp>
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().
Re: Issue tracker housecleaning: SVN-1804
Posted by Yasuhito FUTATSUKI <fu...@poem.co.jp>.
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.
>
> Looking at mailer.py in trunk@HEAD, SMTPOutput.finish() doesn't handle
> any exceptions. In particular:
>
> SMTP.login may raise:
> * SMTPHeloError
> * SMTPAuthenticationError
> * SMTPNotSupportedError
> * SMTPException
>
> SMTP.sendmail may raise:
> * SMTPRecipientsRefused
> * SMTPHeloError
> * SMTPSenderRefused
> * SMTPDataError
> * SMTPNotSupportedError
>
> Any of these exceptions cause the same impact. The same is probably
> true of any unhandled exceptions throughout the script, but SMTP errors
> are completely outside our control.
>
> So we have a choice to make:
>
> Either:
>
> 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.
Cheers,
--
Yasuhito FUTATSUKI <fu...@poem.co.jp>
Re: Issue tracker housecleaning: SVN-1804
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Nathan Hartman wrote on Sun, Oct 27, 2019 at 23:14:55 -0400:
> [[[
> 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.
*lightbulb*
> Found by: rzigweid
> Review by: Daniel Shahaf <d....@daniel.shahaf.name>
> Yasuhito FUTATSUKI <fu...@poem.co.jp>
Committers may be referred to by their names in COMMITTERS, but non-committers
should be referred to by name, when available.
In this case, having checked <http://subversion.tigris.org/issues/show_bug.cgi?id=1804> to
find rzigweid's name,¹ the conventional form would be:
.
Found by: Robert M. Zigweid
Review by: danielsh
futatuki
Email addresses are optional, in the sense that contribulyzer doesn't require
them. When specified, we tend to escape @ as {_AT_}. (This particular
replacement is recognized by tools/dev/contribulyze.py.)
In my review I specifically noted that I hadn't done a complete review. It would
have been appropriate to add a parenthetical recording that fact.
> +++ tools/hook-scripts/mailer/mailer.py (working copy)
> @@ -285,6 +285,7 @@ class SMTPOutput(MailedOutput):
> self.write(self.mail_headers(group, params))
>
> def finish(self):
> + try:
> if self.cfg.is_set('general.smtp_ssl') and self.cfg.general.smtp_ssl
> == 'yes':
Your client inserted a hard line break here. That's not a deal breaker for a
-x-w patch, of course; just a readability thing.
> server = smtplib.SMTP_SSL(self.cfg.general.smtp_hostname)
> else:
> @@ -294,6 +295,9 @@ class SMTPOutput(MailedOutput):
> 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("mailer.py: SMTP error occurred: %s\n" % (detail,))
> + raise MessageSendFailure
Sorry for not catching this in my previous review, but is there any additional
context we provide to the sysadmin who'll read this error message? For
example, the revision number, repository name, recipients, subject line,
message-id?
The new log message looks good (though the glob pattern did gave me pause), but
I haven't reviewed the logic changes.
Cheers,
Daniel
¹ Aside: When we migrated from tigris to jira, we seem to have lost the "tigris
username to fullname" mapping. I suspect we don't have any backup of it, save for
what archive.org's spider may have cached.
Re: Issue tracker housecleaning: SVN-1804
Posted by Nathan Hartman <ha...@gmail.com>.
On Tue, Nov 5, 2019 at 1:19 AM Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> Nathan Hartman wrote on Mon, 04 Nov 2019 20:56 +00:00:
> > So before I rush to change the 'raise .. from' lines, I'll wait until
> > we decide which way to go with the required Python version.
>
> That's certainly one option, though personally, I'd suggest to first
> comment out the «from detail» part of the line, to ensure the script
> works correctly on at least one Python version, before looking into
> adding Python 3 support (either in parallel to, or instead of, Python 2
> support). This way, the script will remain releasable while the discussion
> about supported Python version happens.
>
Done in r1869419.
Also:
On Sat, Nov 2, 2019 at 4:00 AM Yasuhito FUTATSUKI <fu...@poem.co.jp>
wrote:
> > finally:
> > server.quit()
>
> As I mentioned before (but I couldn't tell what I want, sorry),
> smtplib.SMTP.quit() can raise exception. If server.quit() in finally
> block has raised an exception and it isn't caught, it will overwrite
> the exception raised in try block or re-raised in exception block.
>
> The exception on server.quit() itself doesn't affect the result that
> we could send message or not, and there is nothing we can do more
> about this SMTP session. So we can catch it, then safely ignore it or
> at most log its cause as warning, and continue the transaction.
> Even if the cause of exception is fatal, It doesn't directly affect next
> attempt to send message.
>
Forgot to mention it before but that's done in r1869378.
On Mon, Nov 4, 2019 at 1:29 AM Yasuhito FUTATSUKI <fu...@poem.co.jp>
wrote:
> In any case, To get mailer.py to work with Python 3, more fix is needed.
> Our svn.* modules map `char *' to `bytes' object, but current mailer.py
> doesn't
> care bytes vs str problems.
>
It's on my to-do list.
Thanks,
Nathan
Re: Issue tracker housecleaning: SVN-1804
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Nathan Hartman wrote on Mon, 04 Nov 2019 20:56 +00:00:
> So before I rush to change the 'raise .. from' lines, I'll wait until
> we decide which way to go with the required Python version.
That's certainly one option, though personally, I'd suggest to first
comment out the «from detail» part of the line, to ensure the script
works correctly on at least one Python version, before looking into
adding Python 3 support (either in parallel to, or instead of, Python 2
support). This way, the script will remain releasable while the discussion
about supported Python version happens.
Cheers,
Daniel
Re: Issue tracker housecleaning: SVN-1804
Posted by Nathan Hartman <ha...@gmail.com>.
> On Sun, Nov 3, 2019 at 11:50 PM Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> We could do:
>
> try:
> # py3
> eval("raise MessageSendFailure from detail")
> except SyntaxError:
> # py2
> raise MessageSendFailure
Neat trick!
> Or we could just keep the «raise … from» construct in there,
unconditional, and
> bump the script's minimum Python version to 3 on trunk. Analysis:
Hmmm...
I'll commit the 'finally' block fix, since that's a separate
issue entirely and deserves its own commit.
Yasuhito points out that there are other issues to fix to support
Py3 in mailer.py.
So before I rush to change the 'raise .. from' lines, I'll wait until
we decide which way to go with the required Python version.
Nathan
Re: Issue tracker housecleaning: SVN-1804
Posted by Yasuhito FUTATSUKI <fu...@poem.co.jp>.
On 2019/11/04 13:50, Daniel Shahaf wrote:
> Yasuhito FUTATSUKI wrote on Mon, Nov 04, 2019 at 11:13:38 +0900:
>> On 2019/11/04 8:16, Nathan Hartman wrote:
>>> On Sun, Nov 3, 2019 at 12:56 AM Nathan Hartman <ha...@gmail.com>
>>> wrote:
>>
>>> It seems there's another problem.
>>>
>>> Apparently the 'raise .. from' construct is Python 3+. Which means
>>> this breaks the mailer for Python 2.7. I found this when I tested on
>>> Python 2.7 earlier and the script terminated with a syntax error at
>>> 'raise MessageSendFailure from detail'.
>>>
>>> Which do you think is better:
>>>
>>> Remove the 'from detail' and lose the original cause of
>>> MessageSendFailure. MessageSendFailure is meant to be handled and
>>> ignored (possibly only used for outputting additional logging),
>>> allowing the mailer to continue mailing, so this might be acceptable.
>>>
>>> Or, alternately:
>>>
>>> Check which version of Python is running, use 'from detail' only when
>>> Python 3+.
>>
>> The latter can't accomplish with only single script, because syntax
>> error occurs on parse on loading, not on execution. To hide
>> 'from detail' from Python 2 interpreter, we should get it out of
>> the script.
>
> We could do:
>
> try:
> # py3
> eval("raise MessageSendFailure from detail")
> except SyntaxError:
> # py2
> raise MessageSendFailure
Ah, I see.
> Or we could just keep the «raise … from» construct in there, unconditional, and
> bump the script's minimum Python veneed rsion to 3 on trunk. Analysis:
>
> + I assume everyone has a py3 interpreter these days, even people whose distros
> will continue to support py2 past its EOL date.
>
> + trunk will be released in April. That's four months _after_ py2's upstream EOL.
>
> - It's possible that some downstreams run distros that will support py2 past
> its EOL date, have written py2 code that extends mailer.py, and won't have
> gotten around to upgrading their custom extensions to py3 by April.
In any case, To get mailer.py to work with Python 3, more fix is needed.
Our svn.* modules map `char *' to `bytes' object, but current mailer.py doesn't
care bytes vs str problems.
(Though it is not related to this analysis and to the discussion about py2 support.)
Cheers,
--
Yasuhito FUTATSUKI <fu...@poem.co.jp>
Re: Issue tracker housecleaning: SVN-1804
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Yasuhito FUTATSUKI wrote on Mon, Nov 04, 2019 at 11:13:38 +0900:
> On 2019/11/04 8:16, Nathan Hartman wrote:
> > On Sun, Nov 3, 2019 at 12:56 AM Nathan Hartman <ha...@gmail.com>
> > wrote:
>
> > It seems there's another problem.
> >
> > Apparently the 'raise .. from' construct is Python 3+. Which means
> > this breaks the mailer for Python 2.7. I found this when I tested on
> > Python 2.7 earlier and the script terminated with a syntax error at
> > 'raise MessageSendFailure from detail'.
> >
> > Which do you think is better:
> >
> > Remove the 'from detail' and lose the original cause of
> > MessageSendFailure. MessageSendFailure is meant to be handled and
> > ignored (possibly only used for outputting additional logging),
> > allowing the mailer to continue mailing, so this might be acceptable.
> >
> > Or, alternately:
> >
> > Check which version of Python is running, use 'from detail' only when
> > Python 3+.
>
> The latter can't accomplish with only single script, because syntax
> error occurs on parse on loading, not on execution. To hide
> 'from detail' from Python 2 interpreter, we should get it out of
> the script.
We could do:
try:
# py3
eval("raise MessageSendFailure from detail")
except SyntaxError:
# py2
raise MessageSendFailure
Or we could just keep the «raise … from» construct in there, unconditional, and
bump the script's minimum Python version to 3 on trunk. Analysis:
+ I assume everyone has a py3 interpreter these days, even people whose distros
will continue to support py2 past its EOL date.
+ trunk will be released in April. That's four months _after_ py2's upstream EOL.
- It's possible that some downstreams run distros that will support py2 past
its EOL date, have written py2 code that extends mailer.py, and won't have
gotten around to upgrading their custom extensions to py3 by April.
I suppose this boils down to "Do we want to continue to support py2 after its
upstream EOL because some of our downstreams might do so?" — which is being
discussed in another thread.
Cheers,
Daniel
Re: Issue tracker housecleaning: SVN-1804
Posted by Yasuhito FUTATSUKI <fu...@poem.co.jp>.
On 2019/11/04 8:16, Nathan Hartman wrote:
> On Sun, Nov 3, 2019 at 12:56 AM Nathan Hartman <ha...@gmail.com>
> wrote:
> It seems there's another problem.
>
> Apparently the 'raise .. from' construct is Python 3+. Which means
> this breaks the mailer for Python 2.7. I found this when I tested on
> Python 2.7 earlier and the script terminated with a syntax error at
> 'raise MessageSendFailure from detail'.
>
> Which do you think is better:
>
> Remove the 'from detail' and lose the original cause of
> MessageSendFailure. MessageSendFailure is meant to be handled and
> ignored (possibly only used for outputting additional logging),
> allowing the mailer to continue mailing, so this might be acceptable.
>
> Or, alternately:
>
> Check which version of Python is running, use 'from detail' only when
> Python 3+.
The latter can't accomplish with only single script, because syntax
error occurs on parse on loading, not on execution. To hide
'from detail' from Python 2 interpreter, we should get it out of
the script.
(The hunk arround server.quit() looks good to me.)
Cheers,
--
Yasuhito FUTATSUKI <fu...@poem.co.jp>
Re: Issue tracker housecleaning: SVN-1804
Posted by Nathan Hartman <ha...@gmail.com>.
On Sun, Nov 3, 2019 at 12:56 AM Nathan Hartman <ha...@gmail.com>
wrote:
> On Sat, Nov 2, 2019 at 4:00 AM Yasuhito FUTATSUKI <fu...@poem.co.jp>
> wrote:
>
>> <snip>
>>
>> > finally:
>> > server.quit()
>> > ]]]
>>
>> As I mentioned before (but I couldn't tell what I want, sorry),
>> smtplib.SMTP.quit() can raise exception. If server.quit() in finally
>> block has raised an exception and it isn't caught, it will overwrite
>> the exception raised in try block or re-raised in exception block.
>
>
It seems there's another problem.
Apparently the 'raise .. from' construct is Python 3+. Which means
this breaks the mailer for Python 2.7. I found this when I tested on
Python 2.7 earlier and the script terminated with a syntax error at
'raise MessageSendFailure from detail'.
Which do you think is better:
Remove the 'from detail' and lose the original cause of
MessageSendFailure. MessageSendFailure is meant to be handled and
ignored (possibly only used for outputting additional logging),
allowing the mailer to continue mailing, so this might be acceptable.
Or, alternately:
Check which version of Python is running, use 'from detail' only when
Python 3+.
Thanks,
Nathan
[[[
Index: tools/hook-scripts/mailer/mailer.py
===================================================================
--- tools/hook-scripts/mailer/mailer.py (revision 1869339)
+++ tools/hook-scripts/mailer/mailer.py (working copy)
@@ -325,12 +325,12 @@
except smtplib.SMTPRecipientsRefused as detail:
sys.stderr.write("mailer.py: SMTP recipient(s) refused: %s: %s\n"
% (self.to_addrs, detail,))
- raise MessageSendFailure from detail
+ raise MessageSendFailure # from detail <-- breaks on Python 2.7 !!!!
except smtplib.SMTPSenderRefused as detail:
sys.stderr.write("mailer.py: SMTP sender refused: %s: %s\n"
% (self.from_addr, detail,))
- raise MessageSendFailure from detail
+ raise MessageSendFailure # from detail <-- breaks on Python 2.7 !!!!
except smtplib.SMTPException as detail:
# All other errors are fatal; this includes:
@@ -339,7 +339,11 @@
raise
finally:
- server.quit()
+ try:
+ server.quit()
+ except smtplib.SMTPException as detail:
+ sys.stderr.write("mailer.py: Error occurred during SMTP session
cleanup: %s\n"
+ % (detail,))
class StandardOutput(OutputBase):
]]]
Re: Issue tracker housecleaning: SVN-1804
Posted by Nathan Hartman <ha...@gmail.com>.
On Sat, Nov 2, 2019 at 4:00 AM Yasuhito FUTATSUKI <fu...@poem.co.jp>
wrote:
> It's too late, but it is a bit pity....
>
> On 2019/10/30 0:11, Nathan Hartman wrote:
>
> <snip>
> > After incorporating all the preceding feedback, the function is
> > considerably longer, but we handle and report errors:
> > > [[[> def finish(self):
>
> <snip>
>
> > finally:
> > server.quit()
> > ]]]
>
> As I mentioned before (but I couldn't tell what I want, sorry),
> smtplib.SMTP.quit() can raise exception. If server.quit() in finally
> block has raised an exception and it isn't caught, it will overwrite
> the exception raised in try block or re-raised in exception block.
Yes, you did say that before, but I forgot. :-(
I'll fix it soon (when I'll be at my computer).
The exception on server.quit() itself doesn't affect the result that
> we could send message or not, and there is nothing we can do more
> about this SMTP session. So we can catch it, then safely ignore it or
> at most log its cause as warning, and continue the transaction.
> Even if the cause of exception is fatal, It doesn't directly affect next
> attempt to send message.
Ok. Will do.
Thank you for catching that!!
Nathan
Re: Issue tracker housecleaning: SVN-1804
Posted by Yasuhito FUTATSUKI <fu...@poem.co.jp>.
It's too late, but it is a bit pity....
On 2019/10/30 0:11, Nathan Hartman wrote:
<snip>
> After incorporating all the preceding feedback, the function is
> considerably longer, but we handle and report errors:
> > [[[> def finish(self):
<snip>
> finally:
> server.quit()
> ]]]
As I mentioned before (but I couldn't tell what I want, sorry),
smtplib.SMTP.quit() can raise exception. If server.quit() in finally
block has raised an exception and it isn't caught, it will overwrite
the exception raised in try block or re-raised in exception block.
The exception on server.quit() itself doesn't affect the result that
we could send message or not, and there is nothing we can do more
about this SMTP session. So we can catch it, then safely ignore it or
at most log its cause as warning, and continue the transaction.
Even if the cause of exception is fatal, It doesn't directly affect next
attempt to send message.
Cheers,
--
Yasuhito FUTATSUKI <fu...@poem.co.jp>
Re: Issue tracker housecleaning: SVN-1804
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Nathan Hartman wrote on Thu, 31 Oct 2019 03:24 +00:00:
> > SVN-1804: "mailer.py doesn't catch SMTPRecipientsRefused in finish()"
>
> The fix for SVN-1804 is committed in r1869194.
>
> Thanks to all for your feedback (and patience!)
You're welcome, and here's my +0 for backport to 1.13.x.
Cheers,
Daniel
Re: Issue tracker housecleaning: SVN-1804
Posted by Nathan Hartman <ha...@gmail.com>.
> SVN-1804: "mailer.py doesn't catch SMTPRecipientsRefused in finish()"
The fix for SVN-1804 is committed in r1869194.
Thanks to all for your feedback (and patience!)
Re: Issue tracker housecleaning: SVN-1804
Posted by Nathan Hartman <ha...@gmail.com>.
On Tue, Oct 29, 2019 at 11:36 AM Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> Nathan Hartman wrote on Tue, Oct 29, 2019 at 11:11:58 -0400:
> > [[[
> > def finish(self):
> ⋮
> > except smtplib.SMTPRecipientsRefused as detail:
> > sys.stderr.write("mailer.py: SMTP recipient(s) refused: %s: %s\n"
> > % (self.to_addrs, detail,))
> > raise MessageSendFailure
>
> Would «raise MessageSendFailure from detail» make sense here, and in the
> next
> «except» case as well?
>
Yes. Good catch! I'll fix it.
> > 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.
>
> Thanks. HACKING does document this explicitly, too.
It sure does.
I think some of the earlier points you brought up are not yet mentioned in
HACKING. Making a note to look at that more carefully later and fix if
necessary...
Thanks,
Nathan
Re: Issue tracker housecleaning: SVN-1804
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Nathan Hartman wrote on Tue, Oct 29, 2019 at 11:11:58 -0400:
> [[[
> def finish(self):
⋮
> except smtplib.SMTPRecipientsRefused as detail:
> sys.stderr.write("mailer.py: SMTP recipient(s) refused: %s: %s\n"
> % (self.to_addrs, detail,))
> raise MessageSendFailure
Would «raise MessageSendFailure from detail» make sense here, and in the next
«except» case as well?
> Daniel, thank you for finding rzigweid's real name. I didn't know how
> to do that. :-)
You're welcome. For context, Subversion's homepage and issue tracker were both
at https://subversion.tigris.org/ for the first decade of the project's
existence. When we moved to ASF (between 1.6.0 and 1.7.0), we also migrated to
an ASF-hosted issue tracker to avoid the tigris.org bus factor risk. The old
issue tracker was then made read-only.
This is also related to the transition from svn.collab.net to svn.apache.org,
documented in ^/subversion/README (sic).
> 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.
Thanks. HACKING does document this explicitly, too.
Cheers,
Daniel
Re: Issue tracker housecleaning: SVN-1804
Posted by Nathan Hartman <ha...@gmail.com>.
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
Re: Issue tracker housecleaning: SVN-1804
Posted by Branko Čibej <br...@apache.org>.
On 29.10.2019 04:30, Yasuhito FUTATSUKI wrote:
> On 2019-10-28 12:14, Nathan Hartman wrote:
>> Finally got back around to this...
>>
>> With these changes, I verified that:
>> * sending with SMTP works and mailer.py exits with 0 on success
>> * using a deliberately wrong configuration to trigger a failure, the
>> SMTP
>> exception is caught, the message prints to stderr, and mailer.py
>> exits
>> with 1 to indicate failure(s) occurred.
>>
>> Since I didn't send a secondary diff or proper log message before, here
>> it is, with the corrections suggested by Daniel.
>>
>> Currently we don't give different treatment to errors in SMTP.login and
>> SMTP.sendmail, although as Yasuhito points out, this may cause repeated
>> errors. Let me know if you feel this should be addressed.
>
> I worry about lock out caused by repeated helo/ehlo error or
> authentication
> failure when the user uses external SMTP service, watched by inspection
> tools (e.g. fail2ban, etc). So I think it is better that
> SMTPAuthenticationError and SMTPHeloError are treated as fatal.
+1
> Even if only one error caused per run, successive trigger to run the
> script
> can alse cause it, though.
Yes, but there's nothing we can do about that. We can only, as you
suggested, give the administrator some time to avoid the lockout by
failing early on fatal errors.
-- Brane
Re: Issue tracker housecleaning: SVN-1804
Posted by Yasuhito FUTATSUKI <fu...@poem.co.jp>.
On 2019-10-28 12:14, Nathan Hartman wrote:
> Finally got back around to this...
>
> With these changes, I verified that:
> * sending with SMTP works and mailer.py exits with 0 on success
> * using a deliberately wrong configuration to trigger a failure, the SMTP
> exception is caught, the message prints to stderr, and mailer.py exits
> with 1 to indicate failure(s) occurred.
>
> Since I didn't send a secondary diff or proper log message before, here
> it is, with the corrections suggested by Daniel.
>
> Currently we don't give different treatment to errors in SMTP.login and
> SMTP.sendmail, although as Yasuhito points out, this may cause repeated
> errors. Let me know if you feel this should be addressed.
I worry about lock out caused by repeated helo/ehlo error or authentication
failure when the user uses external SMTP service, watched by inspection
tools (e.g. fail2ban, etc). So I think it is better that
SMTPAuthenticationError and SMTPHeloError are treated as fatal.
Even if only one error caused per run, successive trigger to run the script
can alse cause it, though.
> Secondary diff (svn diff -x-wp):
>
> [[[
>
> Index: tools/hook-scripts/mailer/mailer.py
> ===================================================================
> --- tools/hook-scripts/mailer/mailer.py (revision 1869058)
> +++ 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,6 +285,7 @@ class SMTPOutput(MailedOutput):
> self.write(self.mail_headers(group, params))
>
> def finish(self):
> + 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:
> @@ -294,6 +295,9 @@ class SMTPOutput(MailedOutput):
> 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("mailer.py: SMTP error occurred: %s\n" % (detail,))
> + raise MessageSendFailure
I'm sorry I also couldn't catch all to point out.
If server.login() or server.sendmail() raise an exception, server.quit()
will be skipped, so the SMTP connections may be left until Python's GC can
find them. To avoid this, it is better to make finally clause and
if server is valid SMTP object, then do explicitly sever.quit() in it.
Note that smtplib.SMTP_SSL() or smtplib.SMTP() also may raise exception,
`server' is not set and thus server.quit() is not need in such case.
server.quit() itself may raise exception even if server is valid
smtplib.SMTP object, so it may also need another try: ... exception: block.
(To understand what happens when errors occur, SMTP server side log may
also be helpful).
> class StandardOutput(OutputBase):
> @@ -421,11 +425,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 +439,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 +465,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 +496,9 @@ class PropChange(Messenger):
> 'to' : tempfile2.name,
> }))
> self.output.finish()
> + except MessageSendFailure:
> + ret = 1
> + return ret
>
>
> def get_commondir(dirlist):
> @@ -564,7 +578,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 +595,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 +1413,8 @@ class UnknownMappingSpec(Exception):
> pass
> class UnknownSubcommand(Exception):
> pass
> +class MessageSendFailure(Exception):
> + pass
>
>
> if __name__ == '__main__':
> @@ -1455,8 +1476,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
>
> ]]]
I prefer to propagate error by rasing exception rather than return value,
however I could't find this sort of coding convensions here, so it is
just my impression.
Cheers,
--
Yasuhito FUTATSUKI <fu...@poem.co.jp>
Re: Issue tracker housecleaning: SVN-1804
Posted by Nathan Hartman <ha...@gmail.com>.
Finally got back around to this...
With these changes, I verified that:
* sending with SMTP works and mailer.py exits with 0 on success
* using a deliberately wrong configuration to trigger a failure, the SMTP
exception is caught, the message prints to stderr, and mailer.py exits
with 1 to indicate failure(s) occurred.
Since I didn't send a secondary diff or proper log message before, here
it is, with the corrections suggested by Daniel.
Currently we don't give different treatment to errors in SMTP.login and
SMTP.sendmail, although as Yasuhito points out, this may cause repeated
errors. Let me know if you feel this should be addressed.
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 and attempt to continue
execution to avoid losing any later emails. If SMTP error occurs, we
report it to stderr and the script's exit code is nonzero.
* 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): Wrap logic in try .. except block; report
SMTP exception to stderr.
(*.generate): Wrap for loop contents in try .. except block; return
nonzero if SMTP error(s) occurred.
(__main__): Exit nonzero if SMTP error(s) occurred.
Found by: rzigweid
Review by: Daniel Shahaf <d....@daniel.shahaf.name>
Yasuhito FUTATSUKI <fu...@poem.co.jp>
]]]
Secondary diff (svn diff -x-wp):
[[[
Index: tools/hook-scripts/mailer/mailer.py
===================================================================
--- tools/hook-scripts/mailer/mailer.py (revision 1869058)
+++ 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,6 +285,7 @@ class SMTPOutput(MailedOutput):
self.write(self.mail_headers(group, params))
def finish(self):
+ 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:
@@ -294,6 +295,9 @@ class SMTPOutput(MailedOutput):
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("mailer.py: SMTP error occurred: %s\n" % (detail,))
+ raise MessageSendFailure
class StandardOutput(OutputBase):
@@ -421,11 +425,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 +439,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 +465,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 +496,9 @@ class PropChange(Messenger):
'to' : tempfile2.name,
}))
self.output.finish()
+ except MessageSendFailure:
+ ret = 1
+ return ret
def get_commondir(dirlist):
@@ -564,7 +578,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 +595,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 +1413,8 @@ class UnknownMappingSpec(Exception):
pass
class UnknownSubcommand(Exception):
pass
+class MessageSendFailure(Exception):
+ pass
if __name__ == '__main__':
@@ -1455,8 +1476,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
]]]
Thanks for the help!
Nathan
Re: Issue tracker housecleaning: SVN-1804
Posted by Nathan Hartman <ha...@gmail.com>.
On Thu, Oct 24, 2019 at 6:20 PM Yasuhito FUTATSUKI <fu...@poem.co.jp>
wrote:
>
> Exception on SMTP.login will be repeated unless its response code indicates
> temporary error (3XX), because each try uses same parameters. On the other
> side, on SMTP.sendmail, with other sender, recipient, or even message data,
> try after failure may succeed, of course it depend on kind of exception.
> So I think those are not the same impact. Some of them are fatal and
> others are not. I guess that is why the reporter mensions only
> SMTPRecipientRefused in those exceptions.
>
> As how far we analyze and handle errors honestly is a trade-off
> between simplicity and precision, I don't want and don't aim to analyze
> too deep
Thank you to Daniel and Yasuhito for reviewing!
I think we should aim for simplicity, unless you feel strongly that we
should handle the login error separately.
Granted, this is not a perfect solution. Also it does not attempt to
identify and address any other unhandled exceptions that might occur
elsewhere in the program.
But it is an improvement, because right now we have no error handling
whatsoever for SMTP failures.
I'll fix the mistakes when I get back to the office and I'll post a
corrected secondary diff and proper log message...
Thanks again for reviewing.
Nathan
Re: Issue tracker housecleaning: SVN-1804
Posted by Yasuhito FUTATSUKI <fu...@poem.co.jp>.
On 2019/10/25 7:19, Yasuhito FUTATSUKI wrote:
> Exception on SMTP.login will be repeated unless its response code indicates
> temporary error (3XX), because each try uses same parameters. On the other
^^^
Temporary failures are 4XX.
Cheers,
--
Yasuhito FUTATSUKI <fu...@poem.co.jp>
Re: Issue tracker housecleaning: SVN-1804
Posted by Yasuhito FUTATSUKI <fu...@poem.co.jp>.
I'm sorry I didn't point out exeptions caused in smtplib, and
too late to reply.
(I already read
<CA...@mail.gmail.com>
on Wed, 23 Oct 2019 11:48:28 -0400 from Nathan and
<20...@tarpaulin.shahaf.local2>
on Thu, 24 Oct 2019 02:37:17 +0000 from Daniel,
but my this reply is not related to them immediately, so I reply to
<CA...@mail.gmail.com>
on Sun, 20 Oct 2019 00:46:25 -0400 from Nathan again.)
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.
>
> Looking at mailer.py in trunk@HEAD, SMTPOutput.finish() doesn't handle
> any exceptions. In particular:
>
> SMTP.login may raise:
> * SMTPHeloError
> * SMTPAuthenticationError
> * SMTPNotSupportedError
> * SMTPException
>
> SMTP.sendmail may raise:
> * SMTPRecipientsRefused
> * SMTPHeloError
> * SMTPSenderRefused
> * SMTPDataError
> * SMTPNotSupportedError
>
> Any of these exceptions cause the same impact. The same is probably
> true of any unhandled exceptions throughout the script, but SMTP errors
> are completely outside our control.
Exception on SMTP.login will be repeated unless its response code indicates
temporary error (3XX), because each try uses same parameters. On the other
side, on SMTP.sendmail, with other sender, recipient, or even message data,
try after failure may succeed, of course it depend on kind of exception.
So I think those are not the same impact. Some of them are fatal and
others are not. I guess that is why the reporter mensions only
SMTPRecipientRefused in those exceptions.
As how far we analyze and handle errors honestly is a trade-off
between simplicity and precision, I don't want and don't aim to analyze
too deep.
Cheers,
--
Yasuhito FUTATSUKI <fu...@poem.co.jp>