Skip to content

Commit

Permalink
add exception and exc_info to request
Browse files Browse the repository at this point in the history
  • Loading branch information
@mmerickel
mmerickel committed Mar 1, 2016
1 parent 7f1174b commit 68b303a
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions pyramid/view.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -606,6 +606,8 @@ def invoke_exception_view(
# have been mutated by the view, and its state is not
# sane (e.g. caching headers)
with hide_attrs(request, 'exception', 'exc_info', 'response'):
attrs['exception'] = exc_info[0]
attrs['exc_info'] = exc_info
# we use .get instead of .__getitem__ below due to

This comment has been minimized.

Sign in to view

Copy link
@tseaver

tseaver Mar 2, 2016

Member

How are you dealing with the possible cycles involved in saving the traceback to a local?

This comment has been minimized.

Sign in to view

Copy link
@mmerickel

mmerickel Mar 2, 2016

Author Member

I'm open to suggestions! We could put a try/finally around the with-statement and delete the local variable. Am I understanding your concern correctly? I'm aware of the warnings around using sys.exc_info() in the python docs.

This comment has been minimized.

Sign in to view

Copy link
@tseaver

tseaver Mar 2, 2016

Member

We had repeated problems with such uses in Zope-land (huge memory leaks). Maybe we could use the traceback module to save a formatted version of the traceback, instead?

This comment has been minimized.

Sign in to view

Copy link
@mmerickel

mmerickel Mar 2, 2016

Author Member

We didn't make up exc_info... it's something we pass to all exception views already via the exception view tween.

pyramid/pyramid/tweens.py

Line 21 in 74675f7

except Exception as exc:

I can't just get rid of it unless you want to start that as a separate discussion. :-) It'd be good to focus comments here on how to fix the usage of exc_info.

This comment has been minimized.

Sign in to view

Copy link
@digitalresistor

digitalresistor Mar 2, 2016

Member

hide_attrs is going to remove the item from the request anyway and restore the original, if any.

This comment has been minimized.

Sign in to view

Copy link
@mmerickel

mmerickel Mar 2, 2016

Author Member

hide_attrs is going to remove the item from the request anyway and restore the original, if any.

Yeah that doesn't solve the issue with the local reference to the exc_info on the stack frame.
# https://github.com/Pylons/pyramid/issues/700
request_iface = attrs.get('request_iface', IRequest)
Expand Down

1 comment on commit 68b303a

@mmerickel
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a little thought I'm fairly certain the reference cycle isn't possible here. The issue would be when sys.exc_info() is called from within the stack frame that caught the exception. At that point the current stack frame (containing the except) would be referenced by the exc_info var and vice-versa the exc_info references the current stack frame. However since we are inside of invoke_exception_view which is not directly catching the exception - it is a different stack frame that is not on the stack tracked by exc_info. Thus the reference cycle should not occur and exc_info should gc correctly.

def foo():
    try:
        raise RuntimeError
    except:
        exc_info = sys.exc_info() # references current frame
def handle_exc():
    exc_info = sys.exc_info() # should not be circular

def foo():
    try:
        raise RuntimeError
    except:
        handle_exc()

Please sign in to comment.