pb.CopiedFailure.throwExceptionIntoGenerator breaks in Python 2.6.

[sirgolan]

Since the exception type of a CopiedFailure is a string, in Python 2.6, it raises exceptions.TypeError: exceptions must be classes, or instances, not str when CopiedFailure.throwExceptionIntoGenerator is called.

Prior to Python 2.6, it only gave a DeprecationWarning about raising string exceptions.

I'm attaching a test that exposes the issue. When run in Python 2.6, it results in the following:

[ERROR]: twisted.test.test_pb.CopyableFailureTest.test_throwExceptionIntoGenerator

Traceback (most recent call last):
  File "F:\Projects\Twisted\trunk\twisted\test\test_pb.py", line 1920, in test_throwExceptionIntoGenerator
    copy.throwExceptionIntoGenerator(gen)
  File "F:\Projects\Twisted\trunk\twisted\python\failure.py", line 348, in throwExceptionIntoGenerator
    return g.throw(self.type, self.value, self.tb)
exceptions.TypeError: exceptions must be classes, or instances, not str

[sirgolan]

As a side note, the use case for this is code that uses inlineCallbacks and yields a callRemote or other pb deferred call. If the remote side raises an exception, it's passed back through a CopiedFailure which inlineCallbacks sends to the generator with throwExceptionIntoGenerator.

[sirgolan]

Unit test and possible fix.

[sirgolan]

Updated attachment with a possible way to fix the issue. I added a RemoteException class and turn the exception type of the CopiedFailure into one of those and add the original exception type string into the value. This has the unfortunate side effect of changing the exception type from what was previously raised, so it may not be the best solution.

[exarkun]

There's the minor issue of a few existing unit tests that assert that the type attribute of the CopiedFailure is a string with a particular value. That perhaps means we should leave the type attribute alone and move the RemoteException logic you added into an overridden throwExceptionIntoGenerator on CopiedFailure.

As an aside, CopyableFailure and CopiedFailure are somewhat terrible. I think it'd be great if we came up with some way to eliminate them entirely. With the somewhat recently introduced twisted.python.failure._Traceback, it may be possible to construct a regular Failure (albeit not a perfect failure, since _Traceback cannot be raised, only formatted into a string).

The trick is finding a way to introduce this behavior without breaking compatibility with things expecting CopiedFailure and a string type attribute.

Don't take all this to mean that these issues need to be resolved for this ticket. I'm trying to to keep the bigger picture in mind.

Last thing, please add a news fragment for the change as well. Thanks!

[sirgolan]

(In [30715]) Branching to 'copiedfailure-stringexc-4520'

[sirgolan]

(In [30716]) Re #4520 Add StringTypeException and throw it when trying to throw a string exception in Failure.throwExceptionIntoGenerator.

[sirgolan]

(In [30720]) Re #4520 Skip these tests in Python 2.4.

[sirgolan]

(In [30721]) Re #4520 Add topfiles entry.

[sirgolan]

(In [30725]) Re #4520 Move changes into CopiedFailure.

[sirgolan]

(In [30727]) Re #4520 Fix docstring and stop test from running in Python 2.4.

[sirgolan]

(In [30733]) Re #4520 Remove extra line added accidentally in failure.py.

[sirgolan]

Moved this to an overridden throwExceptionIntoGenerator. I also renamed the exception to be more consistent with other exception names in Twisted. Also added a news fragment. I didn't do anything to fix {{CopyableFailure}}} and CopiedFailure in a more general sense. It seems like that should be done in another ticket.

[exarkun]

Thanks for working on this! CopyableFailure and CopiedFailure give me hives. :(

One thing that I can't quite figure out is when CopiedFailure will have a non-string type. The entire Twisted test suite doesn't manage to exercise the last line of CopiedFailure.throwExceptionIntoGenerator. But jelly.unjelly(jelly.jelly(CopyableFailure(AttributeError("foo")))) will produce a CopiedFailure with a non-string type`. But I guess that doesn't realistically mimic PB's usage of these types (even though to me it looks like it is quite realistic).

So I guess the thing to do would be to add one more test for the non-string case of CopiedFailure.throwExceptionIntoGenerator (or somehow completely rule out of that possibility - despite the unjelly(jelly(...)) behavior I mentioned above - and delete the support in that method for the non-string case).

Aside from that:

1. Separate methods from each other with two blank lines
2. Separate classes from each other with three blank lines
3. It would be great if you could add a class docstring to CopiedFailure and document the type/usage of the type attribute at least (and value and tb if you can figure them out too).
4. There are actually some PB failure tests already, they're just hidden in twisted/test/test_pbfailure.py for some reason. I guess these new tests should go in that module as well.

Thanks again!

[Koblaid]

Could the branch possibly be merged to the trunk so it will be in the next release? Having PB exceptions working in Python 2.6 would be great. I would also suggest to increase the priority of this ticket because Twisted already supports Python 2.6.

If someone completely new to Twisted developement could help finsihing this ticket, I would give it a try. (Yeah, TwistedDevelopment is a bit scary for beginners ;-) )

[exarkun]

Addressing the review feedback in comment 13 would be a start. Some of it should be fairly easy, even for a new contributor.

[Koblaid]

I did some work on the ticket. Some tests are still missing, I'll do them tomorrow.

In addition to the feedback from the review I changed some details of the branch. It wasn't merged yet, so I hope thats ok. But maybe someone should review my patch.

My main assumption was that type and traceback of the remote exception will return as strings as PB doesnt allow anything else here. Another thing to note: I added a 'remoteTraceback' attribute to RemoteError because the remote traised can not be raised into a generator (Python limitation).

Here is a changelog:

• added some blank lines
• added docstrings
• RemoteError.remoteString: changed name
• RemoteError.remoteTraceback: for reason see docstring
• RemoteError.value: removed, the value is stored as standard excpetion argument when the exception is raised
• CopiedFailure.throwExceptionIntoGenerator: removed type check, self.type will always be a string

I would appreciate a short feedback to be sure I'm heading in the right direction.

Replying to exarkun:

Thanks for working on this! CopyableFailure and CopiedFailure give me hives. :( One thing that I can't quite figure out is when CopiedFailure will have a non-string type. The entire Twisted test suite doesn't manage to exercise the last line of CopiedFailure.throwExceptionIntoGenerator. But jelly.unjelly(jelly.jelly(CopyableFailure(AttributeError("foo")))) will produce a CopiedFailure with a non-string type`. But I guess that doesn't realistically mimic PB's usage of these types (even though to me it looks like it is quite realistic).

Hmmm... I dont think this case can occur with a real PB connection. I'll check it tomorrow.

So I guess the thing to do would be to add one more test for the non-string case of CopiedFailure.throwExceptionIntoGenerator (or somehow completely rule out of that possibility - despite the unjelly(jelly(...)) behavior I mentioned above - and delete the support in that method for the non-string case). Aside from that: 1. Separate methods from each other with two blank lines

Partly done

1. Separate classes from each other with three blank lines

Partly done (There are differences from the Twisted coding standard all over the code. What should I do? I tried to be consistent with the rest of the file.)

1. It would be great if you could add a class docstring to CopiedFailure and document the type/usage of the type attribute at least (and value and tb if you can figure them out too).

Done

1. There are actually some PB failure tests already, they're just hidden in twisted/test/test_pbfailure.py for some reason. I guess these new tests should go in that module as well.

Will do tomorrow

Thanks again!

[Koblaid]

Diff pb.py (for comment 16)

[Koblaid]

Okay, I've moved the tests to twisted/test/test_pbfailure.py and adjusted the tests to the changes.

I've also checked why jelly.unjelly(jelly.jelly(CopyableFailure(AttributeError("foo")))) produces a non string type. The reason is that CopyableFailure.getStateToCopy() won't get called while jellying the failure in this way. Instead only the __dict__ of the object is copied. This seems to happen if jelly.jelly is called without an invoker.

Is the patch ok? If not I can do further changes. I would be happy if the branch could be merged :-)

[Koblaid]

Debian testing seems to start dropping support for Python 2.5. Currently the zope.interface package is broken, so Twisted is unusable.

Could anybody please do a review?

We use Debian testing on our servers, so we have to patch Twisted with this branch. I would like to return to unchanged Debian packages as soon as possible, so it would be great if this branch could included in the next release of Twisted.

[thijs]

Thanks for your patch, I added the 'review' keyword so it get's noticed (see ReviewProcess).

[glyph]

(In [31725]) Apply latest 'patch_complete.diff' re #4520

[glyph]

 Build results look pretty good, with the usually unfortunate potpourri of random spurious failures.

[glyph]

Reviewing.

[glyph]

Thanks for your work on this! Just a few more points.

1. There are some bits of trailing whitespace introduced in each file, they should be removed.
2. There should be a newline immediately after the triple-quote in new docstrings.
3. The unit test, unfortunately, will still pass if throwExceptionIntoGenerator's implementation is replaced with a simple raise StopIteration. It doesn't actually need to call the code in order to satisfy the test. Tests should generally be written to avoid this, by always making assertions in the body of the test itself, where they can't be missed.
4. The docstring of the test should be written in an active style, describing what the behavior of a given function or method is. Try not to use words like "successfully" or "correctly"; instead, briefly describe what constitutes successful or correct behavior.
5. The usage of a fully-instantiated instance as the "copy" in the test isn't a realistic simulation of copying. In particular, its __init__ will have been invoked, which generally doesn't happen when copying stuff across the wire; so, if getStateToCopy were to change (incorrectly, to not set the traceback) in the future, the test wouldn't fail.
6. CopiedFailure has @param fields - in its class docstring. Classes don't take parameters, functions do, so this doesn't make any sense. Maybe you meant @ivar (instance variable)?
7. C{string} isn't a python type. I think you mean C{str}.

I think I'm going to address this feedback myself, but I think it's just a tad too much (there were functional changes associated with the tests) to just go ahead and merge, so I'll put it back into review in a bit.

[glyph]

(In [31728]) trim trailing whitespace re #4520

[glyph]

(In [31729]) Trim more trailing whitespace re #4520

[glyph]

(In [31730]) re-wrap docstrings and add some minor grammar tweaks re #4520

[glyph]

(In [31731]) do assertions in the test body, not in a callback re #4520

[glyph]

(In [31732]) More accurately simulate what happens when a Copyable traverses the wire. re #4520

[glyph]

(In [31733]) 'string' isn't a type, but 'str' is re #4520

[glyph]

I anticipate that  build results will look good again when they're ready, so I am going to go ahead and put this into review.

[exarkun]

1. In the docstring of CopiedFailure in pb.py, L{flavors.RemoteCopy} should be L{pb.RemoteCopy}
2. The docstring for CopiedFailure.value says it will be a CopiedFailed, but that seems wrong to me. An Exception, I think?
3. There's also some documentation for remoteTraceback on CopiedFailure, but I don't think there's actually a remoteTraceback on CopiedFailure, just on RemoteError.
4. On RemoteError, remoteTraceback and remoteType are documented as params instead of ivars.
5. The first paragraph of the RemoteError docstring doesn't read awesomefully to me. If you can think of any ways in which it might be improved, that would be nice.
6. The news fragment doesn't follow the style guide

[Koblaid]

I'll try to address/fix some of the points.

[Koblaid]

Replying to exarkun:

1. In the docstring of CopiedFailure in pb.py, L{flavors.RemoteCopy} should be L{pb.RemoteCopy}

Hmm, the source for RemoteCopy is in twisted.spread.flavors. But the external API is pb.RemoteCopy, correct? ==> Fixed

2. The docstring for CopiedFailure.value says it will be a CopiedFailed, but that seems wrong to me. An Exception, I think?

If the value attribute of CopyableFailure is a Failure, its class will be switched to CopyableFailure on serialisation (see pb.py, line 442). But it seems that CopyableFailure.value can never be a Failure (see failure.py, line 203). If this is correct, lines 442 and 443 in pb.py are not needed ==> Fixed the docstring

3. There's also some documentation for remoteTraceback on CopiedFailure, but I don't think there's actually a remoteTraceback on CopiedFailure, just on RemoteError.

traceback will be unjellied from CopyableFailure (see pb.py, line 451). RemoteCopy will receive its traceback attribute from CopiedFailure

4. On RemoteError, remoteTraceback and remoteType are documented as params instead of ivars.

==> Fixed

5. The first paragraph of the RemoteError docstring doesn't read awesomefully to me. If you can think of any ways in which it might be improved, that would be nice.

==> Fixed (or at least, tried ;-)

6. The news fragment doesn't follow the style guide

==> Fixed

I'll upload the patch in a second. I think points 3 - 6 are ok, but especially with 2 I'm really not sure. Should the ticket go into the review nevertheless?

[Koblaid]

Patch to response no 34

[exarkun]

(In [32208]) Apply patch-2011-05-09.diff

refs #4520

[exarkun]

Thanks. Sorry for the delay in this review. I applied the patch to the branch, which now looks pretty good, except:

3. There's also some documentation for remoteTraceback on CopiedFailure, but I don't think there's actually a remoteTraceback on CopiedFailure, just on RemoteError.

traceback will be unjellied from CopyableFailure (see pb.py, line 451). RemoteCopy will receive its traceback attribute from CopiedFailure

traceback will be, indeed. But remoteTraceback doesn't exist; I think it's just a mistake in the docstring additions to CopiedFailure, it should be documenting traceback instead of remoteTraceback.

If the value attribute of CopyableFailure is a Failure, its class will be switched to CopyableFailure on serialisation (see pb.py, line 442). But it seems that CopyableFailure.value can never be a Failure (see failure.py, line 203). If this is correct, lines 442 and 443 in pb.py are not needed.

Good catch. I changed line 443 to raise an exception and ran the full test suite and nothing failed. I suspect we should drop that check. On IRC, therve said he thinks this check should be removed too. So I'll just remove it in the branch.

The rest looks good, thanks! I'll  make sure buildbot is happy with the latest code and then merge.

[exarkun]

(In [32211]) Merge copiedfailure-stringexc-4520

Author: sirgolan, Koblaid, glyph Reviewer: exarkun, glyph Fixes: #4520

Allow inlineCallbacks and exceptions raised from a twisted.spread remote call to work together. A new RemoteError exception will be raised into the generator when a yielded Deferred fails with a remote PB failure.