10210 Improve connection error handling in ESMTPSenderFactory #1645

richvdh · 2021-07-28T20:42:29Z

Scope and purpose

This fixes #10210 by passing the failure reason back to the SMTPSenderFactory out-of-band.

Contributor Checklist:

The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/10210
I ran tox -e lint to format my patch to meet the Twisted Coding Standard
I have created a newsfragment in src/twisted/newsfragments/ (see: News files)
The title of the PR starts with the associated Trac ticket number (without the # character).
I have updated the automated tests and checked that all checks for the PR are green.
I have submitted the associated Trac ticket for review by adding the word review to the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.
The merge commit will use the below format
The first line is automatically generated by GitHub based on PR ID and branch name.
The other lines generated by GitHub should be replaced.

Merge pull request #123 from twisted/4356-branch-name-with-trac-id

Author: richvdh
Reviewer: 
Fixes: ticket:10210

Make sure we remember the reason that the connection failed, and raise that in
preference to "connection done" or whatever.

Make sure we remember the reason that the connection failed, and raise that in preference to "connection done" or whatever.

glyph

I don't have time for a full review now but hopefully these initial thoughts are useful!

glyph · 2021-07-29T00:11:29Z

src/twisted/mail/test/test_smtp.py

+            raise Exception("ESMTPSenderFactory unexpectedly completed")
+
+        sentDeferred.addCallbacks(callback, errback)
+        return sentDeferred


In general we now strongly prefer the style of test where you don't spin up a real reactor and do for-real network I/O; instead, consider using the various utilities in twisted.internet.testing to produce deterministic mocks for your network set-up attempts. That said, this style of test is definitely better than nothing, so if you feel that the deterministic version would take you a bunch more time we can work with this for now (it's certainly not the first such test in this file).

Thanks for the speedy initial review, @glyph - and sorry for the slow turnaround on my side.

Yeah, I'm aware of the preference. I initially just went with the flow in this file, but I've just taken a bit of a harder look at what it would take to use twisted.test.iosim.FakeTransport and friends to connect up the client and server.

It would be a bit fiddly, but the real problem is that what I really want to test here is how _newtls.startTLS interacts with the SMTP code, and startTLS relies on wrapping a real FileDescriptor object - or at least, something that behaves much the same as one. Possibly I could rejig FakeTransport enough to make startTLS work with it ... but at that point I start to feel like I'm just testing the tests.

So, as painful as it is, I think this might be a case where we're better off with a real reactor, at least for now.

No need to be sorry for a slow turnaround. We're all very time-constrained here in open source land, and we do things when we can.

src/twisted/mail/test/test_smtp.py

glyph

Thanks so much for improving this error handling. I hope that this feedback is minor and you an address so we can land soon?

glyph · 2022-11-29T00:38:56Z

src/twisted/mail/smtp.py

+        # If the initial connection succeeded, but there was a later error
+        # (such as TLS validation failure), we're more interested in that than
+        # "Connection was closed cleanly" or whatever.
+        if self.currentProtocol and self.currentProtocol.failureReason:


We don't want to test self.currentProtocol.__bool__ here, we just want to check if it is not None

I find that inline ternaries are more expressive than overloading boolean logic for tests like this, so, for clarity, I think I'd prefer this:

Suggested change

if self.currentProtocol and self.currentProtocol.failureReason:

if self.currentProtocol.failureReason is not None if self.currentProtocol is not None else False:

The first point is semantic, although the ternary thing is just stylistic, so feel free to skip it if you'd prefer … is not None and … is not None

Honestly I find the ternary expression utterly incomprehensible, but I've corrected this to check for Noneness.

glyph · 2022-11-29T00:42:00Z

src/twisted/mail/test/test_smtp.py

+        Attempting to connect to an ESMTP server which presents an invalid certificate
+        should produce a meaningful error.


Love this docstring, very close to https://jml.io/pages/test-docstrings.html .

(It would be even nicer if it gave a crisper definition of "meaningful" but that is really nitpicking)

glyph · 2022-11-29T00:45:14Z

src/twisted/mail/test/test_smtp.py

+        return self.assertFailure(sentDeferred, smtp.SMTPConnectError).addCallback(
+            checkVerifyFailedMessage
+        )


FYI, this sort of real-networking test that returns a Deferred is really not the right way to test protocol-parsing logic like this; it does unnecessarily non-deterministic I/O with the operating system. If you can, it should be rewritten to be more robust using twisted.test.iosim.IOPump or similar. At the very least however, please remove the direct call to listenTCP and instead use the higher-level self.loopback(...) to at least abstract out the connection-making logic.

Yeah. As I wrote before, I struggled to get this to work with iosim because startTLS wants to wrap a real file descriptor.

I had a look at making it work with self.loopback. It could be my failure to understand how to use loopback, but as far as I can tell, it wants to build its own ClientFactory (a LoopbackClientFactory), which seems like it makes it impossible to instantiate an SMTPSenderFactory (which is where the interesting changes are here).

@exarkun says

You mentioned that there are problems using iosim because startTLS expects a "real life file descriptor"? iosim has built-in support for startTLS though, so I wonder what happened there.

src/twisted/test/test_amp.py tests AMP's startTLS usage and uses iosim
any of the tests that use connectedServerAndClient

glyph · 2022-11-29T00:47:07Z

Oops. I see i already mostly gave this review before. I wonder why the robot did not remove the needs-review label?

glyph

Hello robot. Please see comment review for requested changes.

adiroiban · 2022-11-29T08:56:05Z

@glyph

Oops. I see i already mostly gave this review before. I wonder why the robot did not remove the needs-review label?

You only left a review with an "indecisive comment"
The robot only updates the label on "approved" or "request changes"

glyph

Next time escape the robot command in that comment @adiroiban 😉

glyph · 2022-12-31T16:01:24Z

please review

richvdh · 2023-07-28T12:06:32Z

Sadly it's not looking like I'm going to have much time to work on this in the near future. My dayjob has taken me away from regular work on Twisted-based projects, and to be honest I've pretty much forgotten all the state I had on this when I worked on it two years ago.

I'd really love it if this could be merged as-is, despite the less-than-ideal test, since IMHO it fixes a real bug that bites users. That said, I do understand I'm not the one that will have to live with the headache of maintaining that test in future.

Anyway, in short, if anyone else has time to pick this up and see if the tests can be improved, it would be amazing.

glyph · 2023-07-28T15:47:19Z

@richvdh Thanks for clarifying the status here. No worries that you don't have time to volunteer, it is, after all, volunteering :)

glyph

I think I agree with the reasoning of landing this even with a sub-optimal test. Whoever comes along to land it, please:

fix up the public name to be private
clear the exception when it's propagated to a Deferred
file a follow-up to fix the test

glyph · 2024-01-08T23:59:59Z

src/twisted/mail/smtp.py

@@ -950,6 +951,9 @@ def __init__(self, identity, logsize=10):
        self.code = -1
        self.log = util.LineLog(logsize)

+        # the reason we were unable to successfully send the message, if any.
+        self.failureReason: Optional[Failure] = None


This requires an @ivar if it's going to be made public, but I would suggest making it private, since it is exposed publicly at the relevant time in self.result.errback(...) below.

src/twisted/mail/smtp.py

src/twisted/mail/test/test_smtp.py

src/twisted/mail/smtp.py

glyph · 2024-01-09T00:11:10Z

src/twisted/mail/smtp.py

+        exn = SMTPConnectError(
+            -1, "Unable to connect to SMTP server: " + str(err.value)
+        )
+        exn.__cause__ = err.value


I appreciate the use of __cause__ to chain the exception properly; we should probably make this a bit easier to do with Failure.

src/twisted/mail/test/test_smtp.py

Co-authored-by: Glyph <code@glyph.im>

richvdh added 3 commits July 28, 2021 21:04

Improve connection error handling in ESMTPSenderFactory

ccdf708

Make sure we remember the reason that the connection failed, and raise that in preference to "connection done" or whatever.

news

0667215

lint

94a783a

richvdh changed the title ~~[10210] Improve connection error handling in ESMTPSenderFactory~~ 10210 Improve connection error handling in ESMTPSenderFactory Jul 28, 2021

glyph reviewed Jul 29, 2021

View reviewed changes

Use assertFailed to avoid coverage error

6619a0e

richvdh force-pushed the smtp_exceptions branch from ca3d15d to 6619a0e Compare August 3, 2021 21:35

richvdh and others added 4 commits August 3, 2021 22:46

lint

378998d

Merge remote-tracking branch 'origin/trunk' into smtp_exceptions

e150e5e

Merge remote-tracking branch 'origin/trunk' into smtp_exceptions

e2b5a69

Merge branch 'trunk' into smtp_exceptions

4f8c4ca

adiroiban requested a review from a team July 8, 2022 09:59

chevah-robot added the needs-review label Jul 8, 2022

Merge branch 'trunk' into smtp_exceptions

c054a9b

glyph reviewed Nov 29, 2022

View reviewed changes

Merge branch 'trunk' into smtp_exceptions

24ed281

chevah-robot requested a review from a team November 29, 2022 00:47

glyph requested changes Nov 29, 2022

View reviewed changes

chevah-robot added needs-changes and removed needs-review labels Nov 29, 2022

chevah-robot added needs-review and removed needs-changes labels Nov 29, 2022

chevah-robot requested a review from a team November 29, 2022 08:56

glyph requested changes Nov 29, 2022

View reviewed changes

chevah-robot added needs-changes and removed needs-review labels Nov 29, 2022

check for None, not falsiness

be05b02

richvdh requested a review from glyph December 28, 2022 21:01

Merge branch 'trunk' into smtp_exceptions

e9362a5

chevah-robot added needs-review and removed needs-changes labels Dec 31, 2022

chevah-robot requested a review from a team December 31, 2022 16:01

glyph removed their request for review December 31, 2022 16:01

glyph added 2 commits February 13, 2023 22:33

Merge branch 'trunk' into smtp_exceptions

c98662c

Merge branch 'trunk' into smtp_exceptions

31f84af

glyph added 2 commits September 13, 2023 13:00

Merge branch 'trunk' into smtp_exceptions

55c3513

Merge branch 'trunk' into smtp_exceptions

a66dd70

glyph approved these changes Jan 9, 2024

View reviewed changes

chevah-robot added needs-merge and removed needs-review labels Jan 9, 2024

Merge branch 'trunk' into smtp_exceptions

c467a42

adiroiban reviewed Jan 12, 2024

View reviewed changes

src/twisted/mail/test/test_smtp.py Outdated Show resolved Hide resolved

Apply suggestions from code review

3866012

Co-authored-by: Glyph <code@glyph.im>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

10210 Improve connection error handling in ESMTPSenderFactory #1645

10210 Improve connection error handling in ESMTPSenderFactory #1645

richvdh commented Jul 28, 2021 •

edited

glyph left a comment

glyph Jul 29, 2021

richvdh Aug 3, 2021

glyph Aug 30, 2021

glyph left a comment

glyph Nov 29, 2022

richvdh Dec 28, 2022

glyph Nov 29, 2022

glyph Nov 29, 2022

richvdh Dec 28, 2022

richvdh May 23, 2023

glyph commented Nov 29, 2022

glyph left a comment

adiroiban commented Nov 29, 2022

glyph left a comment

glyph commented Dec 31, 2022

richvdh commented Jul 28, 2023

glyph commented Jul 28, 2023

glyph left a comment

glyph Jan 8, 2024

glyph Jan 9, 2024

	if self.currentProtocol and self.currentProtocol.failureReason:
	if self.currentProtocol.failureReason is not None if self.currentProtocol is not None else False:

		Attempting to connect to an ESMTP server which presents an invalid certificate
		should produce a meaningful error.

10210 Improve connection error handling in ESMTPSenderFactory #1645

Are you sure you want to change the base?

10210 Improve connection error handling in ESMTPSenderFactory #1645

Conversation

richvdh commented Jul 28, 2021 • edited

Scope and purpose

Contributor Checklist:

glyph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glyph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glyph commented Nov 29, 2022

glyph left a comment

Choose a reason for hiding this comment

adiroiban commented Nov 29, 2022

glyph left a comment

Choose a reason for hiding this comment

glyph commented Dec 31, 2022

richvdh commented Jul 28, 2023

glyph commented Jul 28, 2023

glyph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Jul 28, 2021 •

edited