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

Rack responses may be frozen #40598

Closed
wants to merge 1 commit into from
Closed

Conversation

sj26
Copy link
Contributor

@sj26 sj26 commented Nov 11, 2020

Summary

We are getting errors like this in our production rails app from this middleware:

 FrozenError: can't modify frozen Hash
 .../actionpack-6.0.3.4/lib/action_dispatch/middleware/request_id.rb:27:in `block in call'

I'm not sure where the frozen response is coming from, but it seems like a thing which can happen -- even within rails, in ActionDispatch::Http::Response#before_sending:

def before_sending
  # Normally we've already committed by now, but it's possible
  # (e.g., if the controller action tries to read back its own
  # response) to get here before that. In that case, we must force
  # an "early" commit: we're about to freeze the headers, so this is
  # our last chance.
  commit! unless committed?

  headers.freeze
  request.commit_cookie_jar! unless committed?
end

This middleware seems to take pains to avoid too much object churn, so the spirit has been followed here to only duplicate what is required to insert the header.

We are getting errors like this in our production rails app from this
middleware:

     FrozenError: can't modify frozen Hash

I'm not sure where the frozen response is coming from, but it seems like
a thing which can happen -- even within rails, in
ActionDispatch::Http::Response#before_sending:

    def before_sending
      # Normally we've already committed by now, but it's possible
      # (e.g., if the controller action tries to read back its own
      # response) to get here before that. In that case, we must force
      # an "early" commit: we're about to freeze the headers, so this is
      # our last chance.
      commit! unless committed?

      headers.freeze
      request.commit_cookie_jar! unless committed?
    end

This middleware seems to take pains to avoid too much object churn, so
the spirit has been followed here to only duplicate what is required to
insert the header.
@rails-bot rails-bot bot added the actionpack label Nov 11, 2020
@rafaelfranca
Copy link
Member

I think we need to understand why the response and the headers are frozen, otherwise we will be playing whack-a-mole with middleware to handle frozen response. Can you please investigate?

@sj26
Copy link
Contributor Author

sj26 commented Nov 11, 2020

I think I found a fallback response we are sending which is deeply frozen because it is a constant, and so we don't want it mutated because it might affect future requests and responses. This is also a pattern I've seen in a variety of applications and middleware.

I'll make it deep dup the response for now.

But the rack spec doesn't say that responses shouldn't be frozen — is that expected?

@rafaelfranca
Copy link
Member

rack/rack#1597 is related to this. The SPEC will require unfrozen hashes for headers

@sj26
Copy link
Contributor Author

sj26 commented Nov 11, 2020

No worries, sounds like doing a deep dup is the right approach. Thanks!

@sj26 sj26 closed this Nov 11, 2020
@sj26 sj26 deleted the request-id-headers-frozen branch November 11, 2020 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants