[Twisted-Python] Re: Hanging test cases (Was: Evangelism notes...)

[Bob Ippolito]

On May 6, 2005, at 6:51 PM, David Bolen wrote:

> Bob Ippolito <bob at redivi.com> writes:
>
> with respect to deferredResult/deferredError:
>
>
>> Sorry, but Jp is right.  They have no place in any code.  Their
>> existence is a mistake and should be corrected ASAP.  They severely
>> violate the reactor interface.
>>
>
> Hmm, then perhaps it's the reactor interface that could use some
> improving rather than removing this high level functionality?  Or is
> everyone just saying that the implementation (versus the concept) of
> the current functions is done wrong.  Or maybe I'm just missing what
> major mess having this sort of interface is exposing.

The reactor interface is not what needs improving, it's the way that  
these functions work.  They iterate the reactor when the reactor is  
in a stopped state.  The reactor should either be running, or not  
running.  There are basically two fundamental issues:

(1) Reactors can only be (meaningfully/predictably/etc) iterated if  
Twisted rules the universe AND the implementation of that reactor is  
amenable to that feature.  This is not a tautology.
(2) Reactors need to fire various startup/shutdown events.  Reactors  
shouldn't be doing ANYTHING unless they are in a running state.

The current deferredResult/deferredError breaks both of these  
conditions.
(1) It iterates the reactor (which is a historically public, but  
conceptually broken interface)
(2) It iterates the reactor in a STOPPED state.  The reactor is never  
"running" during these tests.  Startup/Shutdown does not happen!

The fact that the doc strings talk about re-entrancy of certain  
reactor functions and whatnot scares the shit out of me.  They should  
not have to be re-entrant.  What their current implementation is  
doing is really really broken.

The problem, for tests, is that using a properly written  
deferredResult / deferredError the reactor would startup/shutdown  
violently throughout the course of a single test, and will break the  
hell out of it if it has anything to do with services/etc.  So, while  
for SOME deferreds it would work fine, but for others, it wouldn't.   
Basically, unless you can encapsulate the entire test in a single  
deferred, then it shouldn't be using deferredResult/deferredError.

Therefore, what SHOULD happen is that trial should let you write  
tests that return deferreds, and it should let you write it in the  
deferredGenerator style (so it doesn't suck so much).

Trial *could*, in theory, put the reactor into a "started" state at  
the beginning of every tests and a "stopped" state at the end, but  
then you're testing in a strange environment that doesn't really  
mimic how Twisted actually works, and it still breaks (1) which makes  
it unsuitable for testing the reactors where Twisted does not rule  
the universe.  Testing this theory would require changes to the  
reactor interface to make some parts of if re-entrant (bleargh).

> Given Glyph's recent response, I might also clarify that while our
> heaviest use of these functions are within unit tests, we aren't using
> trial, so any linkage between poor behavior of these implementations
> and trial is not something I'm referring to or worry about breaking.
>
> What I do think is that there is a very good and practical reason to
> be able to unwind deferrable operations into a blocking structure for
> a wide variety of test conditions, and losing that capability would
> make writing tested Twisted code (at least for our situation) much
> more fragile and error prone (the tests themselves).  And that's not
> quite the same as the newer waitForDeferred stuff in 2.0 that
> integrates with generators.  While slick, it still doesn't work as
> well for easily maintainable tests in many of our situations.

Well, it causes you to write two lines of code instead of one.  That  
sucks, but so what?  You don't have to write big callback chains..   
It's conceptually identical, it's just that you have to throw in some  
extra boiler plate that says THIS IS A DEFERRED.  If the way we say  
that wasn't so ugly, it might actually be a good thing from a  
readability standpoint :)

I'm not *entirely* sure why "waitForDeferred" is really necessary at  
all, though "deferredGenerator" certainly is.  In the interest of not- 
wearing-off-your-fingerprints one might write a "deferredGenerator"  
specifically for trial (or maybe in general) that just makes sure  
you're yielding a Deferred.

I *think* that it does this so that it can assume the last thing you  
yield is the result.  I would say that you should wrap the result  
instead of wrap every intermediate thing, since the intermediate  
things you do in such a deferred generator should far outnumber the  
times that you return from it.  Perhaps whoever wrote it had a good  
reason that I just don't understand, but when I wrote something  
equivalent a few years ago I wrote it such that there wasn't quite so  
much boilerplate.

> For example, in our system we may very heavy use of our own components
> all of which implement their entire API via deferrable interfaces.
> Interacting with multiple such components during a test is thus a
> multi-stage deferrable process.  Having these blocking calls permits a
> test method to say something like:
>
>     self.user = deferredResult(self.umgr.user())
>     self.user.email = 'test at dom.ain'
>     deferredResult(self.umgr.saveUser(self.user))

# let's assume I've aliased waitForDeferred to "wait"

d = waitForDeferred(self.umgr.user())
yield d
self.user = d.getResult()
self.user.email = 'test at dom.ain'
d = waitForDeferred(self.umgr.saveUser(self.user))
yield d
d.getResult()

So, the test is 6 lines instead of 3.  Which sucks, but it's  
correct.  For tests that do more stuff, it probably should be even  
less of a problem.

If Python grows an extended iteration protocol, where yield becomes  
an expression, you could make it 3 lines again.  This might actually  
happen in Python 2.5.

> Sure, I could write the user() and saveUser() calls with a callback
> chain, and then I'd have to nest the call to saveUser based on the
> user callback, probably something like:
>
>     def fail(failure):
>         self.fail(failure)
>
>     def gotUser(user):
>         self.user = user
>         self.user.email = 'test at dom.ain'
>         return self.umgr.saveUser(self.user)
>
>     d = self.umgr.user()
>     d.addCallback(gotUser)
>     d.addErrback(fail)
>
> but I see no advantage (*in the context of such a test*) to doing it
> this way, and I see a real loss of clarity and maintainability for the
> test itself.

Well, let's say your database thing is a service, that maintains some  
kind of ephemeral state that's required in some way.  If  
deferredResult were properly written, this ephemeral state would hit  
the bit bucket on each deferredResult, probably breaking your code  
even though the test are "correct".

If you add such a component to the system, you might have to rewrite  
all of your tests, because it would not be possible to fix the way  
deferredResult works.

> Since each component in our system has a deferrable interface, any
> test which is going to interact with multiple components would quickly
> evolve a fairly deep callback system in the test, without much benefit
> that I can see.

So, use deferredGenerator / waitForDeferred.

-bob