On 28/04/2021 07:06, Glyph wrote:
Is the SMTP code holding the Factory
wrong? Or is it reasonable to expect the verification error to
propagate into clientConnectionFailed - in which case, how could this
work?
Thanks for your thoughts!
Hi Richard,
Sorry for the delayed response here.
This is a bug in Twisted, and I think it boils down to this line:
https://github.com/twisted/twisted/blob/3c868ac11786eef7ea269caa3056f00854128957/src/twisted/protocols/tls.py#L391
The code as written here appears to be expecting this sequence:
1. failVerification is called with a reason containing a helpful
OpenSSL verification error
2. we save that reason as `self._reason` for reporting later, calling
abortConnection()
3. since the connection got aborted, we expect our underlying transport
to call loseConnection on us
4. we will then get a falsey reason [?!?!] and as such we will use
self._reason instead
Assumption 4 is nonsense and has never been true within Twisted as far
as I know; connectionLost always gets a Failure, Failures are never
falsey, so we will never use self._reason. To fix this we need to
actually honor self._reason under the conditions we expect, i.e. it's a
ConnectionAborted
https://github.com/twisted/twisted/blob/3c868ac11786eef7ea269caa3056f00854128957/src/twisted/internet/error.py#L209
Thanks very much for your thoughts on this, Glyph - it's always helpful
to have an insight into the intended design when trying to resolve this
sort of thing.
I don't follow your reasoning though. I think you might have misread the
line you point to
(https://github.com/twisted/twisted/blob/3c868ac11786eef7ea269caa3056f00854128957/src/twisted/protocols/tls.py#L391).
It is "self._reason or reason" - ie, if self._reason is already set, it
will take precedence over reason.
In my tests at least, TLSMemoryBIOProtocol.connectionLost is doing
exactly the right thing - it is called with an unhelpful reason, but
substitutes back in the helpful reason which has already been stashed.
Rather, the problem, as I see it, is that it's not
TLSMemoryBIOProtocol.connectionLost that calls
Factory.clientConnectionLost. That is done by tcp.Client.connectionLost,
via one of tcp.Client's myriad of base classes, at
https://github.com/twisted/twisted/blob/3c868ac11786eef7ea269caa3056f00854128957/src/twisted/internet/tcp.py#L508.
Of course, that doesn't get the benefit of TLSMemoryBIOProtocol's reason
switcheroo.
I'm still not quite sure who is in the wrong here.
Cheers
Richard
PS: yes, once we figure out what's going wrong here, I'll at least write
up an issue...
_______________________________________________
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
