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/3c868ac11786eef7ea269caa3056f0085412...
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/3c868ac11786eef7ea269caa3056f0085412...
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/3c868ac11786eef7ea269caa3056f0085412...). 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/3c868ac11786eef7ea269caa3056f0085412.... 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...