Avoid force encoding on frozen strings

[bvicenzo]

using the gem Twitter, that uses this gem, we're receiving the bellow error:

This would have happened on this PR when the strings inside of gem have became frozen also, and the problem was solved making test strings unfrozen.

This PR proposes to make the Response class deal with frozen strings.

[ixti]

thanks for the PR i'm gonna merge it together with the one that changes the api to be more io alike asap

[caifara]

We're running into this too, any chance on a quick release?

[tarcieri]

Can you confirm the problem is fixed? If so, I can cut a release

[caifara]

That's really great! I'm quite ashamed to first ask a fast release only to see the frozen version in our dependencies to be quite a bit older. That said, the mentioned error and changes in this PR point to the exact same problem and the source doesn't seem to have changed much on that point.

Another problem is that we see the error in production sporadically, but not in development. That means there's no straightforward way to be sure the problem is really fixed.

And yet another problem is that I don't really know why the solution proposed in this PR is really helping. Where is the bug in the original code? I can only trigger the bug locally by forcing @contents.freeze in the while loop. As you don't run a release immediately I'm guessing you too aren't entirely sure?

[tarcieri]

Correct, I'm not sure either, and this looks like it will allocate two strings for every read, which will increase GC pressure. In that regard it's probably not the greatest solution.

[tarcieri]

A better strategy is probably:

1. Check if string matches @encoding. If it does, we're done.
2. Check if string is frozen. If it isn't, use #force_encoding(@encoding)
3. If the string is frozen, allocate a new string with the given encoding

[cooljeanius]

This caused sferik/t-ruby#452; how do I update its dependency on this to include the fix? I tried something in sferik/t-ruby#462 but I'm not sure if it's right... (I don't actually speak ruby)

[tarcieri]

It was released in v5.0.2 in August of last year.

You need to update your Gemfile.lock, e.g. bundle update

[janko]

Correct, I'm not sure either, and this looks like it will allocate two strings for every read, which will increase GC pressure. In that regard it's probably not the greatest solution.

Yeah, the benefit of the previous String#clear implementation was that it deallocates chunks immediately, instead of having to wait for the GC to collect them. So, after this change, HTTP::Response::Body#to_s allocates 2x more memory than it did previously for responses with large bodies (e.g. images).

We have a background job where we need to download many images in a row, and it's currently getting OOM-killed. Deallocating content immediately after its no longer would likely help prevent the memory usage from growing too rapidly, allowing time for GC. I wanted to use Body#to_s for downloading images, but now image chunks will remain in memory that I cannot String#clear, so I'll need to use Body#readpartial instead.