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

Return a Rack::BodyProxy from the Rails::Rack::Logger monkey patch #333

Merged
merged 5 commits into from
Jan 24, 2022

Conversation

ghiculescu
Copy link
Contributor

@ghiculescu ghiculescu commented Aug 11, 2021

@iloveitaly
Copy link
Collaborator

@ghiculescu could you rebase on master and add an entry to the changelog?

@ghiculescu
Copy link
Contributor Author

Done, did you also want commits squashed or are you fine to use the github squasher?

@iloveitaly
Copy link
Collaborator

@ghiculescu looks like CI is failing, could you take another look?

No worries about squashing—I'll let the GH merge tool handle that.

@iloveitaly iloveitaly merged commit 7ef493d into roidrage:master Jan 24, 2022
@ghiculescu ghiculescu deleted the patch-1 branch January 24, 2022 19:34
@sigra
Copy link

sigra commented Jan 25, 2022

@iloveitaly @ghiculescu 👋

I'm not sure, but it looks like Rack::BodyProxy#close always calls block in ensure part. But in this PR the block is nil, so I have Read: #<NoMethodError: undefined method :call for nil:NilClass> for each request. It doesn't affect user work, but this spam to stdout doesn't seem right.

Check the original implementation in Rails — https://github.com/rails/rails/blob/v6.1.4.4/railties/lib/rails/rack/logger.rb#L38. They use block.

Also, I checked all gems in my gemset and found out that ALL of them use block.
image

P.S. I've copied block logic from original Rails implementation and it solves my issue — Salesap@d7ec60e

@iloveitaly
Copy link
Collaborator

@sigra interesting! Mind submitting a PR to change our use of BodyProxy?

ghiculescu added a commit to ghiculescu/lograge that referenced this pull request Jan 25, 2022
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.

3 participants