Opened 14 years ago

Closed 14 years ago

Last modified 5 years ago

#3132 release blocker: regression closed fixed (fixed)

Failure creation fails if the exception was generated from a C extension

Reported by: arkanes Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: arkanes, therve Branch: branches/failure-c-extension-3132
branch-diff, diff-cov, branch-cov, buildbot
Author: therve

Description (last modified by therve)

This may only apply to Pyrex/Cython extensions, I haven't been able to test fully. The problem is at line 366 in failure.py, where the last frames code is inspecting looking for a yield opcode. If the previous frame was a call to a C extension, the co_code is '', and this generates and index error. Below is a traceback:

Traceback (most recent call last):
  File "c:\Python25\lib\site-packages\twisted\python\threadpool.py", line 161, i
n _worker
    context.call(ctx, function, *args, **kwargs)
  File "C:\Python25\lib\site-packages\twisted\python\context.py", line 59, in ca
llWithContext
    return self.currentContext().callWithContext(ctx, func, *args, **kw)
  File "C:\Python25\lib\site-packages\twisted\python\context.py", line 37, in ca
llWithContext
    return func(*args,**kw)
  File "c:\Python25\lib\site-packages\twisted\internet\threads.py", line 26, in
_putResultInDeferred
    f = failure.Failure()
  File "C:\Python25\lib\site-packages\twisted\python\failure.py", line 168, in _
_init__
    exc_value = self._findFailure()
  File "C:\Python25\lib\site-packages\twisted\python\failure.py", line 366, in _
findFailure
    if lastFrame.f_code.co_code[lastTb.tb_lasti] != cls._yieldOpcode:
exceptions.IndexError: string index out of range

Attachments (6)

raiser.pyx (180 bytes) - added by arkanes 14 years ago.
Pyrex extension for testing error
setup.py (396 bytes) - added by arkanes 14 years ago.
Build script for extension
test_failure.patch (1.1 KB) - added by arkanes 14 years ago.
Patch for test_failure adding a unit test for the bug
failure.patch (540 bytes) - added by arkanes 14 years ago.
patch with proposed fix
raiser.c (14.5 KB) - added by arkanes 14 years ago.
raiser.c
updated_per_review.patch (2.4 KB) - added by arkanes 14 years ago.
updated patch as per review

Download all attachments as: .zip

Change History (23)

Changed 14 years ago by arkanes

Attachment: raiser.pyx added

Pyrex extension for testing error

Changed 14 years ago by arkanes

Attachment: setup.py added

Build script for extension

Changed 14 years ago by arkanes

Attachment: test_failure.patch added

Patch for test_failure adding a unit test for the bug

Changed 14 years ago by arkanes

Attachment: failure.patch added

patch with proposed fix

comment:1 Changed 14 years ago by arkanes

Keywords: inlineCallbacks failure removed

comment:2 Changed 14 years ago by arkanes

Keywords: review added

comment:3 Changed 14 years ago by therve

Description: modified (diff)

comment:4 Changed 14 years ago by therve

author: therve
Branch: branches/failure-c-extension-3132

(In [23114]) Branching to 'failure-c-extension-3132'

comment:5 Changed 14 years ago by therve

(In [23115]) Check patch in.

Refs #3132

comment:6 Changed 14 years ago by therve

Owner: Glyph deleted

comment:7 Changed 14 years ago by radix

Cc: arkanes therve added
Owner: set to arkanes

[1] Why is it a cdef class instead of just a function?

[2] the docstring of raiser.pyx is not formatted properly. Also, it'd be nice if it referred to test_failure.

[3] I guess I would be slightly happier if you actually introspected something about the failure in the test, but then, the change isn't really about what the failure looks like, just that it doesn't blow up horribly.

[4] Please add a comment explaining why we check for an empty co_code, and why it's ok to return when it's empty (because we know that those C extensions can't yield...)

Thanks very much, arkanes, for getting this going, and thanks, therve, for organizing it into a branch. I'm reassigning this back to arkanes, but please feel free to deal with the comments if you want, therve :-) I'd like to get this merged so that I can do 8.0.2 very soon.

comment:8 Changed 14 years ago by radix

Keywords: review removed

comment:9 in reply to:  8 Changed 14 years ago by arkanes

Replying to radix:

new patch attached, against Therves branch. I can't get an SVN checkout here (need an http-accessible repo), so I generated it manually and the patch might be a little screwey. Sorry :(.

raiser.c is attached separately so it doesn't bloat the patch up.

Changed 14 years ago by arkanes

Attachment: raiser.c added

raiser.c

Changed 14 years ago by arkanes

Attachment: updated_per_review.patch added

updated patch as per review

comment:10 Changed 14 years ago by arkanes

Keywords: review added

comment:11 Changed 14 years ago by arkanes

Owner: arkanes deleted

comment:12 Changed 14 years ago by radix

Type: defectregression

comment:13 Changed 14 years ago by therve

(In [23269]) Apply patch from tracker, with some docstrings.

Refs #3132

comment:14 Changed 14 years ago by therve

Checked in the branch.

comment:15 Changed 14 years ago by radix

Resolution: fixed
Status: newclosed

(In [23279]) Merge failure-c-extension-3132

Author: arkanes Reviewer: radix Santa's Little Helper: therve Fixes #3132

Constructing failures in contexts where Pyrex or Cython has inserted dodgy pseudo-frames onto the stack now works. This involved a fix to the code which detected existing failures on the exception stack to chain which was trying to introspect those frames.

comment:16 Changed 11 years ago by <automation>

comment:17 Changed 5 years ago by hawkowl

Keywords: review removed

[mass edit] Removing review from closed tickets.

Note: See TracTickets for help on using tickets.