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

Catch exceptions, raise Plug.Conn.WrapperError #278

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

vincedevendra
Copy link

@vincedevendra vincedevendra commented Jan 30, 2023

We have a Phoenix application that expects the [:phoenix, :endpoint, :stop] event to be executed, but I noticed that this was not happening when uncaught exceptions originated from this plug.

This is because Plug.Telemetry registers this event by using Plug.Conn.register_before_send, which, in turn, adds a key to conn.private.

When there is an uncaught exception, Phoenix catches the error to render a standard error response here. When this sends the response, the telemetry callback is run and the event is executed.

Notice that if there is a Plug.Conn.WrapperError, the conn from it is used, but if not, conn is what's in scope at the time the RenderErrors plug runs. This happens to be at the top of the middleware stack before Plug.Telemetry registers the callback. Thus, no [:phoenix, :endpoint, :stop] event is executed.

Note also that bug reporting plugs may also expect request parsers etc. to have surfaced important bits of info on the conn (we ran into this issue as well, but worked around it).

This issue can be solved by rescuing errors in call and using Plug.Conn.Wrapper.reraise/4

@vincedevendra vincedevendra changed the title Rescue exceptions, raise Plug.Conn.WrapperError Catch exceptions, raise Plug.Conn.WrapperError Jan 30, 2023
@vincedevendra
Copy link
Author

vincedevendra commented Jan 31, 2023

I should note that this likely only effects Phoenix applications without a router, since Phoenix Router wraps errors. As our application is solely a GraphQL server, we don't have/need a router. That's in line with the advice given here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant