python 3.5/3.8 compatibilty fix: need to pass host and port at init time to the SMTP session object by digiturtle · Pull Request #92 · marrow/mailer

digiturtle · 2020-09-26T11:08:32Z

The new version of smtplib requires an hostname at init time. Also if the mail provider uses a non-standard port (as an example, iCloud uses port 587) and the port is not passed at init time, marrow.mailer will fail with a "connection refused" error (errno 61)

…ime to the SMTP session object

amcgregor

I'm unsure of the actual need for this change. Could evidence in the form of the traceback you are encountering (an MCVE would be even better!) and/or documentation as to the nature of the required change be provided? Additionally, there appears to be unnecessary duplication/branches that can be removed, and there is a subtle negative behavior difference (impacting diagnostic logging capability) as a result of this change. Thank you.

amcgregor · 2020-09-26T17:22:09Z

-            connection = SMTP_SSL(local_hostname=self.local_hostname, keyfile=self.keyfile,
-                                  certfile=self.certfile, timeout=self.timeout)
+            if PY35_OR_LATER:
+                connection = SMTP_SSL(local_hostname=self.local_hostname, keyfile=self.keyfile,
+                                      certfile=self.certfile, timeout=self.timeout)
+            else:
+                connection = SMTP_SSL(local_hostname=self.local_hostname, keyfile=self.keyfile,
+                                      certfile=self.certfile, timeout=self.timeout)


Why is there a conditional branch here? The invocations are identical.

you're right there is an error in the second commit, sorry about that, the second commit was added because the travis continuous integration "tricked" me in thinking the first one was breaking python 2 and python < 3.5, but then looking in detail I saw that what seems broken is the continuous integration for some versions of Python. So you should really only pull the first commit. Would you prefer I close this pull request and submit another one?

No need; an additional commit is no worries. Squash merges are nice. 😉

amcgregor · 2020-09-26T17:23:51Z

-            connection = SMTP(local_hostname=self.local_hostname, timeout=self.timeout)
+            if PY35_OR_LATER:
+                connection = SMTP(self.host, port=self.port, local_hostname=self.local_hostname, timeout=self.timeout)
+            else:
+                connection = SMTP(local_hostname=self.local_hostname, timeout=self.timeout)


smtplib has supported passing in the host and port via the SMTP and SMTP_SSL classes as far back as currently available documentation will go, so there should be no need for this behavior to be conditional between versions, at all, unless there is some other deficit I'm not aware of. I would recommend, for some limited consistency, passing host and port positionally. The other arguments deserve more "explanation" (context).

If passed in here, the subsequent connect() call does not need to happen. The logic within SMTP's constructor states that if a host is provided, a connection should be immediately attempted. The reason this was not utilized previously is that it makes it impossible to set the debug level prior to the connection attempt.

You're right again, I have only now noticed that I was using a version that had been patched (probably because of #83) and was already providing a host to the SMTP constructor... in this case it is evident that also a port should be provided. So in the end really this pull request is about issue #83. I have verified that if I remove the patch marrow.mailer doesn't fail with non-standard ports (using python 2.7)

This is the traceback with the exception when using python 3.8 and marrow.mailer without the patch:

connect: ('smtp.mail.me.com', 587) connect: to ('smtp.mail.me.com', 587) None reply: b'220 iCloud SMTP - pv50p00im-tydg10011801.me.com 3.4.7 (2007B172-3902c1cbb426)\r\n' reply: retcode (220); Msg: b'iCloud SMTP - pv50p00im-tydg10011801.me.com 3.4.7 (2007B172-3902c1cbb426)' connect: b'iCloud SMTP - pv50p00im-tydg10011801.me.com 3.4.7 (2007B172-3902c1cbb426)' send: 'ehlo ftoso-mbp.local\r\n' reply: b'250-pv50p00im-tydg10011801.me.com\r\n' reply: b'250-PIPELINING\r\n' reply: b'250-SIZE 28311552\r\n' reply: b'250-ETRN\r\n' reply: b'250-STARTTLS\r\n' reply: b'250-ENHANCEDSTATUSCODES\r\n' reply: b'250-8BITMIME\r\n' reply: b'250-DSN\r\n' reply: b'250 CHUNKING\r\n' reply: retcode (250); Msg: b'pv50p00im-tydg10011801.me.com\nPIPELINING\nSIZE 28311552\nETRN\nSTARTTLS\nENHANCEDSTATUSCODES\n8BITMIME\nDSN\nCHUNKING' send: 'STARTTLS\r\n' reply: b'220 2.0.0 Ready to start TLS\r\n' reply: retcode (220); Msg: b'2.0.0 Ready to start TLS' Delivery of message <160115213980.50096.5092007329413341917@ftoso-mbp.local> failed. Traceback (most recent call last): File "/Users/ftoso/develop/konga/third_party/generated/python38/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/marrow.mailer-4.0.3-py3.8.egg/marrow/mailer/manager/util.py", line 50, in __enter__ transport = pool.transports.get(False) File "/Users/ftoso/develop/konga/third_party/generated/python38/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/queue.py", line 167, in get raise Empty _queue.Empty

During handling of the above exception, another exception occurred:

Traceback (most recent call last): File "send_email_message_mm.py", line 138, in <module> mailer.send(message) File "/Users/ftoso/develop/konga/third_party/generated/python38/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/marrow.mailer-4.0.3-py3.8.egg/marrow/mailer/__init__.py", line 149, in send result = self.manager.deliver(message) File "/Users/ftoso/develop/konga/third_party/generated/python38/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/marrow.mailer-4.0.3-py3.8.egg/marrow/mailer/manager/immediate.py", line 41, in deliver with self.transport() as transport: File "/Users/ftoso/develop/konga/third_party/generated/python38/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/marrow.mailer-4.0.3-py3.8.egg/marrow/mailer/manager/util.py", line 57, in __enter__ transport.startup() File "/Users/ftoso/develop/konga/third_party/generated/python38/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/marrow.mailer-4.0.3-py3.8.egg/marrow/mailer/transport/smtp.py", line 50, in startup self.connect_to_server() File "/Users/ftoso/develop/konga/third_party/generated/python38/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/marrow.mailer-4.0.3-py3.8.egg/marrow/mailer/transport/smtp.py", line 84, in connect_to_server connection.starttls(self.keyfile, self.certfile) File "/Users/ftoso/develop/konga/third_party/generated/python38/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/smtplib.py", line 774, in starttls self.sock = context.wrap_socket(self.sock, File "/Users/ftoso/develop/konga/third_party/generated/python38/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/ssl.py", line 500, in wrap_socket return self.sslsocket_class._create( File "/Users/ftoso/develop/konga/third_party/generated/python38/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/ssl.py", line 1031, in _create self._sslobj = self._context._wrap_socket( ValueError: server_hostname cannot be an empty string or start with a leading dot.

amcgregor · 2020-09-26T17:32:29Z


 log = __import__('logging').getLogger(__name__)

+PY35_OR_LATER = (sys.version_info >= (3,5))


There seems to be no change attributed to Python 3.5 within the documentation:

https://docs.python.org/3.5/library/smtplib.html#smtplib.SMTP
https://docs.python.org/3.8/library/smtplib.html?highlight=3.5#smtplib.SMTP

https://docs.python.org/3.5/library/smtplib.html#smtplib.SMTP_SSL
https://docs.python.org/3.8/library/smtplib.html?highlight=3.5#smtplib.SMTP_SSL

https://docs.python.org/3.5/library/smtplib.html#smtplib.SMTP.connect
https://docs.python.org/3.8/library/smtplib.html?highlight=3.5#smtplib.SMTP.connect

There is a deprecation in how the acceptable certificate context should be properly configured, but this does not seem to relate?

amcgregor · 2020-09-28T02:56:57Z

As per #91 (comment) this is a regression in Python standard library behaviour and should be reported/corrected upstream.

ftoso added 2 commits September 26, 2020 12:23

python 3.7/3.8 compatibilty fix: need to pass host and port at init t…

28ca526

…ime to the SMTP session object

added Python 2 and 3.4 compatibility

3b718f4

amcgregor requested changes Sep 26, 2020

View reviewed changes

amcgregor closed this Sep 28, 2020

amcgregor linked an issue Oct 1, 2020 that may be closed by this pull request

SMTP regression and failure during TLS negotiation under Python 3.7. #83

Open

amcgregor added the meta:rejected This does not seem right and/or will not be worked on. label Oct 1, 2020

amcgregor mentioned this pull request Dec 4, 2020

bpo-36094: Update smtplib.py python/cpython#23635

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

python 3.5/3.8 compatibilty fix: need to pass host and port at init time to the SMTP session object #92

python 3.5/3.8 compatibilty fix: need to pass host and port at init time to the SMTP session object #92
digiturtle wants to merge 2 commits into
marrow:developfrom
digiturtle:develop

digiturtle commented Sep 26, 2020

Uh oh!

amcgregor left a comment •

edited

Loading

Uh oh!

amcgregor Sep 26, 2020

Uh oh!

digiturtle Sep 26, 2020

Uh oh!

amcgregor Sep 26, 2020

Uh oh!

amcgregor Sep 26, 2020 •

edited

Loading

Uh oh!

digiturtle Sep 26, 2020 •

edited

Loading

Uh oh!

digiturtle Sep 26, 2020 •

edited by amcgregor

Loading

Uh oh!

amcgregor Sep 26, 2020

Uh oh!

amcgregor commented Sep 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		log = __import__('logging').getLogger(__name__)

		PY35_OR_LATER = (sys.version_info >= (3,5))

Conversation

digiturtle commented Sep 26, 2020

Uh oh!

amcgregor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcgregor Sep 26, 2020

Choose a reason for hiding this comment

Uh oh!

digiturtle Sep 26, 2020

Choose a reason for hiding this comment

Uh oh!

amcgregor Sep 26, 2020

Choose a reason for hiding this comment

Uh oh!

amcgregor Sep 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

digiturtle Sep 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

digiturtle Sep 26, 2020 • edited by amcgregor Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcgregor Sep 26, 2020

Choose a reason for hiding this comment

Uh oh!

amcgregor commented Sep 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

amcgregor left a comment •

edited

Loading

amcgregor Sep 26, 2020 •

edited

Loading

digiturtle Sep 26, 2020 •

edited

Loading

digiturtle Sep 26, 2020 •

edited by amcgregor

Loading