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

Clear error page state before each request #448

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

felixbuenemann
Copy link
Contributor

This clears out the @error_page state before each request to avoid
dirtying the error state of subsequent requests. For example a name
error in the controller will cause an inspect call on the controller
instance that will then also inspect the @error_page ivar referenced
through the rack env in the controller. One possible side-effect of
this are excessive database queries for unloaded relations.

This also should reduce memory usage, since the app can garbage collect
the no longer referenced state, delete temp files, etc.

For more info see the discussion on: rails/rails#38445

This clears out the @error_page state before each request to avoid
dirtying the error state of subsequent requests. For example a name
error in the controller will cause an inspect call on the controller
instance that will then also inspect the @error_page ivar referenced
through the rack env in the controller. One possible side-effect of
this are excessive database queries for unloaded relations.

This also should reduce memory usage, since the app can garbage collect
the no longer referenced state, delete temp files, etc.
@felixbuenemann
Copy link
Contributor Author

CI errors are unrelated, see #450.

@RobinDaugherty
Copy link
Member

Thanks for the contribution @felixbuenemann!

Holding the most recent error seems to be the intended behavior for Better Errors. By clearing this before every request, it becomes impossible to inspect the most recent error if any other request is made subsequently. For example, when a series of XHRs are made to the application, and one of them fails, you're able to inspect the last failure by visiting /__better_errors. But that will no longer work with your change.

@felixbuenemann
Copy link
Contributor Author

I didn't think about that. I think what would work is to swap the ivar out to a local var and restore it in the else block of the rescue.

@RobinDaugherty
Copy link
Member

I think what would work is to swap the ivar out to a local var and restore it in the else block of the rescue.

I'm not sure what you mean by that. Can you say more?

@felixbuenemann
Copy link
Contributor Author

felixbuenemann commented Mar 8, 2020

I think what would work is to swap the ivar out to a local var and restore it in the else block of the rescue.

I'm not sure what you mean by that. Can you say more?

Sorry, missed the comment. What I mean is that we can store the @error_page instance variable to a local variable before each request in the middleware and restore it in the else block of the begin / rescue / else / end. That way it should not show up in the inspect output.

This should work, since @error_pagegets assigned the new error in the rescue case, so we only need to keep the old error, if no error occurs.

This is of course a bit racy, since you could hit /__better_errors while another request is ongoing, bit that is likely more of a theoretical problem.

Another option would be using something like a thread local, but it wouldn't work if the development server is running multiple threads. That could be worked around by using a global variable instead…

@RobinDaugherty
Copy link
Member

@felixbuenemann I can see why this might clean up the state (without freeing anything of course since @error_page's value would simply be elsewhere.

But can you help me understand what this is solving? You mentioned

That way it should not show up in the inspect output.

But I haven't been able to find a way to inspect the middleware stack in a way that the instance variables are shown. Can you show me what you're seeing?

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.

2 participants