[Twisted-Python] SMTPConnectError as a side effect on connection lost
I hit a strange behavior in which the clone connection error from one test is raised as the error for another test. The tests are not written using trial, but I have thousands of other tests and I have never seen this behaviour. I tried to put a self-contained example at https://gist.github.com/adiroiban/edc0776e3337d0bd3f093aa0f2819deb#file-test... you will need to run it in a virtulenv as it will pull quite a few of dependencies. I have traced the error to this code https://github.com/twisted/twisted/blob/03dcdfb5933c4f83ce6aad3f4bdf080cda65... if err.check(error.ConnectionDone): err.value = smtp.SMTPConnectError( -1, "Unable to connect to server.") self.result.errback(err.value) It looks to me like the initial failure (err) is hijacked here and its errors will never be cleared. If I am doing something like if err.check(error.ConnectionDone): value = smtp.SMTPConnectError( -1, "Unable to connect to server.") else: value = err.value err.cleanFailure() self.result.errback(value) then the test will pass. But I still don't understand how the error from one client request is passed to another client request when they are using different servers. Do you see anything suspicious with this code? Thanks! -- Adi Roiban
On Nov 25, 2017, at 8:52 PM, Adi Roiban <adi@roiban.ro> wrote:
Do you see anything suspicious with this code?
Just that it looks like it leaves a Deferred in an error state. You said the tests weren’t “written using” trial, but are you running them with trial, or with any runner that will consider a logged failure to be a failure? If so, the failure won’t be logged until the garbage collector runs, which might be in a later test.
On 26 November 2017 at 05:36, Glyph <glyph@twistedmatrix.com> wrote:
On Nov 25, 2017, at 8:52 PM, Adi Roiban <adi@roiban.ro> wrote:
Do you see anything suspicious with this code?
Just that it looks like it leaves a Deferred in an error state. You said the tests weren’t “written using” trial, but are you running them with trial, or with any runner that will consider a logged failure to be a failure? If so, the failure won’t be logged until the garbage collector runs, which might be in a later test.
Many thanks for looking into that. I dag deeper and I found the error. The root cause is twisted.internet.posixbase._DisconnectSelectableMixin._disconnectSelectable It starts as: def _disconnectSelectable(self, selectable, why, isRead, faildict={ error.ConnectionDone: failure.Failure(error.ConnectionDone()), error.ConnectionLost: failure.Failure(error.ConnectionLost()) }): """ Utility function for disconnecting a selectable. Supports half-close notification, isRead should be boolean indicating whether error resulted from doRead(). """ self.removeReader(selectable) f = faildict.get(why.__class__) Then `f` is returned but in STMP we have: f.value = smtp.SMTPConnectError( -1, "Unable to connect to server.") This replaces the value for `faildict` via f. Any future call will return that replaced error. I have created a ticket for this https://twistedmatrix.com/trac/ticket/9339 I guess that we can continue the discussion on how to fix it as part of that ticket. ---------- Here is my answer to Glyph's question regarding unhandled failed deferred. The tests are not inheriting from the Trial test case. I can run them with trial, including with --force-gc and no leftover deferred is detected. The SMTP test is [OK]. If I reverse the order, so that the STMP tests is executed last, trial will execute all tests without any error. Thanks! -- Adi Roiban
On Nov 26, 2017, at 5:10 AM, Adi Roiban <adi@roiban.ro> wrote:
On 26 November 2017 at 05:36, Glyph <glyph@twistedmatrix.com> wrote:
On Nov 25, 2017, at 8:52 PM, Adi Roiban <adi@roiban.ro> wrote:
Do you see anything suspicious with this code?
Just that it looks like it leaves a Deferred in an error state. You said the tests weren’t “written using” trial, but are you running them with trial, or with any runner that will consider a logged failure to be a failure? If so, the failure won’t be logged until the garbage collector runs, which might be in a later test.
Many thanks for looking into that.
I dag deeper and I found the error.
Oops, looks like I was totally off base :-).
The root cause is twisted.internet.posixbase._DisconnectSelectableMixin._disconnectSelectable
It starts as:
def _disconnectSelectable(self, selectable, why, isRead, faildict={ error.ConnectionDone: failure.Failure(error.ConnectionDone()), error.ConnectionLost: failure.Failure(error.ConnectionLost()) }): """ Utility function for disconnecting a selectable.
Supports half-close notification, isRead should be boolean indicating whether error resulted from doRead(). """ self.removeReader(selectable) f = faildict.get(why.__class__)
Then `f` is returned but in STMP we have:
f.value = smtp.SMTPConnectError( -1, "Unable to connect to server.")
This replaces the value for `faildict` via f. Any future call will return that replaced error.
Yeah, this is definitely wrong. I don't think it occurred to anyone that someone might set the `value` attribute of an existing Failure. More on the ticket.
I have created a ticket for this
https://twistedmatrix.com/trac/ticket/9339
I guess that we can continue the discussion on how to fix it as part of that ticket.
----------
Here is my answer to Glyph's question regarding unhandled failed deferred.
The tests are not inheriting from the Trial test case.
I can run them with trial, including with --force-gc and no leftover deferred is detected. The SMTP test is [OK].
If I reverse the order, so that the STMP tests is executed last, trial will execute all tests without any error.
Yeah, your diagnosis is correct, mine was wrong. -glyph
participants (2)
-
Adi Roiban -
Glyph