Dont call each when calling body on response to fix #23964#24029
Conversation
|
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
bb3ec11 to
cf5c01b
Compare
cf5c01b to
3ba4f6e
Compare
There was a problem hiding this comment.
We can't remove the memoization here, this makes the body method potentially very expensive, and would break streaming responses.
There was a problem hiding this comment.
@sgrif tests passed locally with the memoization added back. Travis should pick that change up momentarily.
|
After looking at this a bit more, I'm not entirely comfortable with this approach. The |
|
@sgrif are you sure? The only change here is that calling |
|
The issue is that you can now leave the response in an invalid state simply by calling |
|
@sgrif I beleive that's why we got rid of the memoization that was used when calling #body (which rails is not calling internally, except in tests). |
|
You're misunderstanding me. You cannot call |
|
@sgrif can you think of a way to show with a test that a user calling |
7ec481f to
bc2c72f
Compare
- Adds #each_chunk to ActionDispatch::Response. it's a method which
will be called by ActionDispatch::Response#each.
- Make Response#each a proper method instead of delegating to @stream
- In Live, instead of overriding #each, override #each_chunk.
- `#each` should just spit out @str_body if it's already set
- Adds #test_set_header_after_read_body_during_action
to prove this fixes rails#23964
- Adds #test_each_isnt_called_if_str_body_is_written to
ensure #each_chunk is not called when @str_body is available
- Call `@response.sent!` in AC::TestCase's #perform so a test response
acts a bit more like a real response. Makes test that call `#assert_stream_closed`
pass again.
- Additionally assert `#committed?` in `#assert_stream_closed`
- Make test that was calling @response.stream.each pass again by
calling @response.each instead.
26b191e to
b43158a
Compare
|
Is this blocking Rails 5.0.0 release candidate? Is it blocking beta4? |
|
Blocking RC, yes. Not blocking beta 4 On Sun, Apr 24, 2016, 3:12 PM Jeremy Daer notifications@github.com wrote:
|
|
@sgrif I'm under the impression that my latest has addressed your concerns. Also, I did add the tests I mentioned. Just trying to make I'm on the same page as everyone else. |
|
I will review later this week, thanks On Sun, Apr 24, 2016, 8:24 PM Ryan T. Hosford notifications@github.com
|
| private | ||
|
|
||
| def each_chunk(&block) | ||
| @buf.each(&block) # extract into own method |
|
OK after a second review I think I have a better understanding. Thanks for working on this |
|
❤️ |
#each_chunkto ActionDispatch::Response. It's a method which will be called byActionDispatch::Response#each.Response#eacha proper method instead of delegating to@stream#each, override#each_chunk.#eachshould just spit out@str_bodyif it's already set.#test_set_header_after_read_body_during_actionto prove this fixes Accessingresponse.bodyinside any controller action messes up the response #23964.#test_each_isnt_called_if_str_body_is_writtento ensure#eachis not called when@str_bodyis available.@response.sent!in AC::TestCase's#performso a test response acts a bit more like a real response. This makes tests that call#assert_stream_closedpass again.#committed?in#assert_stream_closed.@response.stream.eachpass again by calling@response.eachinstead.