Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

request.invoke exception view #2393

Merged
merged 19 commits into from Mar 14, 2016

Conversation

Projects
None yet
4 participants
@mmerickel
Copy link
Member

mmerickel commented Mar 3, 2016

Introduce a new API for invoking exception views manually. This can be done for several reasons but the primary use-case is for error handling inside of tweens where it is desirable to utilize the registered exception views of the pyramid app to render errors instead of defining some other hook for rendering the error as is required when using wsgi middleware.

@tseaver

This comment has been minimized.

Copy link
Member

tseaver commented on pyramid/view.py in 68b303a Mar 2, 2016

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

This comment has been minimized.

Copy link
Member Author

mmerickel replied Mar 2, 2016

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.

Copy link
Member

tseaver replied Mar 2, 2016

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.

Copy link
Member Author

mmerickel replied Mar 2, 2016

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

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.

Copy link
Member

bertjwregeer replied Mar 2, 2016

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

This comment has been minimized.

Copy link
Member Author

mmerickel replied Mar 2, 2016

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.

@mmerickel

This comment has been minimized.

Copy link
Member Author

mmerickel commented on 68b303a Mar 3, 2016

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()

@mmerickel mmerickel force-pushed the feature.invoke_exception_view branch from c3076fc to bc09250 Mar 4, 2016

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Mar 4, 2016

Much excitement =)

👍 from me. I just gave it a cursory glance though.

@mmerickel

This comment has been minimized.

Copy link
Member Author

mmerickel commented Mar 9, 2016

Hey @tseaver I'm about to merge this, do you have any final objections?

@tseaver

This comment has been minimized.

Copy link
Member

tseaver commented Mar 9, 2016

I guess I'm -0 on checking anything in that leaves the traceback-local binding around until GC kicks in. Not enough to block it (and we may be doing it elsewhere), but it still makes me uncomfortable.

@mmerickel

This comment has been minimized.

Copy link
Member Author

mmerickel commented Mar 9, 2016

AFAICT based on #2393 (comment) there should be no need for GC here as there are no circular refs. The only possible issue in this api is if a user themselves calls sys.exc_info() and passes it to me but at that point they're responsible for handling that - I can't stop them.

mmerickel added a commit that referenced this pull request Mar 14, 2016

@mmerickel mmerickel merged commit 375250d into master Mar 14, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mmerickel mmerickel deleted the feature.invoke_exception_view branch Mar 14, 2016

mmerickel added a commit that referenced this pull request Mar 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.