You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Yasuhito FUTATSUKI <fu...@poem.co.jp> on 2019/11/02 08:00:26 UTC
Re: Issue tracker housecleaning: SVN-1804
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 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