launchpad-dev team mailing list archive

[Andrew Bennetts <andrew.bennetts@xxxxxxxxxxxxx>]

John Arbash Meinel wrote:
[…]
> Basically what I've found is that twisted.python.failure.Failure is
> instantiated when a Deferred raises an Exception. And Failure scales
> O(state).
[…]
> But in bzrlib, and certainly in Launchpad exceptions are often used for
> signaling. They certainly don't signal fatal conditions on every exception.
> 
> I don't think it is very tasteful to hack Twisted to supress the 'tb'
> parameter. Certainly the code isn't written to be modular in that
> respect. All the walking is done directly in the __init__ function, so
> it can't be simply overridden.

As Jono says it'd be wonderful to improve this in upstream Twisted, it's
something that would impact many Twisted apps to some degree.  Perhaps
it would be ok to treat the _debugInfo flag that already exists as the
flag to set whether or not to capture tracebacks in errbacks?  That
might be a bit too all-or-nothing, although it's possible that the
“expensive” work _debugInfo currently does is actually much cheaper than
the regular cleanFailure calls.  It's certainly easy to make a patch to
do that, so I'll do so and see what the other Twisted devs think.  Maybe
it'll inspire someone to think of a better solution.

Another option, perhaps easier in the short term, is to avoid this
problem in codehosting. Failure(exception_instance) doesn't set self.tb,
so an approach would be to explicitly change our codehosting
callbacks/errbacks to:

1. prefer “return Failure(SomeException(…))” over “raise *
   SomeException(…)” and/or

2. wrap callback/errback functions in a decorator to make failures from
   raised errors the cheap way, e.g.:

      def cheap_failures_please(func):
          def wrapped(*args, **kwargs):
	      try:
	          return func(*args, **kwargs)
	      except BaseException, e:
	          return Failure(e)
          return wrapped
      
      d.addCallback(cheap_failures_please(my_callback))
      
At a glance it wouldn't be too outrageously messy to make those changes
to lib/lp/codehosting/vfs/transport.py, so I'll give it a go and propose
a patch to lp too.  Aside from having traceback-less Failures it should
have no change to semantics.

-Andrew.