Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11112021:
Treat 0 returned from HttpStream::ReadResponseBody correctly. (Closed)
|
Created:
8 years, 2 months ago by pivanof Modified:
8 years, 2 months ago Reviewers:
mmenke CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, willchan no longer on Chromium Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionTreat 0 returned from HttpStream::ReadResponseBody correctly.
According to comment to HttpStream::ReadResponseBody() 0 means end of response
while HttpResponseBodyDrainer treated it as error.
BUG=154712
TEST=net_unittests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162415
Patch Set 1 #
Total comments: 2
Patch Set 2 : Clean sync/async separation. #
Total comments: 7
Patch Set 3 : #
Total comments: 1
Patch Set 4 : #
Messages
Total messages: 17 (0 generated)
Looks good, but have a suggestion that I think would make the code a little simpler. http://codereview.chromium.org/11112021/diff/1/net/http/http_response_body_dr... File net/http/http_response_body_drainer_unittest.cc (right): http://codereview.chromium.org/11112021/diff/1/net/http/http_response_body_dr... net/http/http_response_body_drainer_unittest.cc:134: void DelayEofChunk() { delay_eof_chunk_ = true; } Think the logic would be a little simpler if you made this "is_final_chunk_zero_bytes_" and changed behavior accordingly. Then the program flow would be a bit more consistent in both cases. Note that for unknown reasons, the final chunk is read synchronously, so you'd currently need to put the code in the fall through case (where "return buf_len;" is). http://codereview.chromium.org/11112021/diff/1/net/http/http_response_body_dr... net/http/http_response_body_drainer_unittest.cc:156: DCHECK(buf); While you're here, could you turn these into CHECKs? DCHECKs don't make any sense in test code. Thanks.
On 2012/10/12 18:55:05, Matt Menke wrote: > Looks good, but have a suggestion that I think would make the code a little > simpler. > > http://codereview.chromium.org/11112021/diff/1/net/http/http_response_body_dr... > File net/http/http_response_body_drainer_unittest.cc (right): > > http://codereview.chromium.org/11112021/diff/1/net/http/http_response_body_dr... > net/http/http_response_body_drainer_unittest.cc:134: void DelayEofChunk() { > delay_eof_chunk_ = true; } > Think the logic would be a little simpler if you made this > "is_final_chunk_zero_bytes_" and changed behavior accordingly. Then the program > flow would be a bit more consistent in both cases. I'm not sure it will be simpler, but more consistent for sure. I'll try to do that. > Note that for unknown reasons, the final chunk is read synchronously, so you'd > currently need to put the code in the fall through case (where "return buf_len;" > is). I guess in this case my suspicions are correct and I actually need to remove the unused code on the async pass (num_chunks_ never actually becomes 0 in there). But maybe it's better to make consistent and make all chunks assynchronous?
On 2012/10/12 19:02:05, pivanof wrote: > On 2012/10/12 18:55:05, Matt Menke wrote: > > Looks good, but have a suggestion that I think would make the code a little > > simpler. > > > > > http://codereview.chromium.org/11112021/diff/1/net/http/http_response_body_dr... > > File net/http/http_response_body_drainer_unittest.cc (right): > > > > > http://codereview.chromium.org/11112021/diff/1/net/http/http_response_body_dr... > > net/http/http_response_body_drainer_unittest.cc:134: void DelayEofChunk() { > > delay_eof_chunk_ = true; } > > Think the logic would be a little simpler if you made this > > "is_final_chunk_zero_bytes_" and changed behavior accordingly. Then the > program > > flow would be a bit more consistent in both cases. > > I'm not sure it will be simpler, but more consistent for sure. I'll try to do > that. > > > Note that for unknown reasons, the final chunk is read synchronously, so you'd > > currently need to put the code in the fall through case (where "return > buf_len;" > > is). > > I guess in this case my suspicions are correct and I actually need to remove the > unused code on the async pass (num_chunks_ never actually becomes 0 in there). > But maybe it's better to make consistent and make all chunks assynchronous? My thought was to add an is_sync_ bool and have it default to false. And then add in a test or two that are sync - and to make the existing tests completely async. If you want, you can go ahead do that in this CL.
On 2012/10/12 19:04:42, Matt Menke wrote: > On 2012/10/12 19:02:05, pivanof wrote: > > On 2012/10/12 18:55:05, Matt Menke wrote: > > > Looks good, but have a suggestion that I think would make the code a little > > > simpler. > > > > > > > > > http://codereview.chromium.org/11112021/diff/1/net/http/http_response_body_dr... > > > File net/http/http_response_body_drainer_unittest.cc (right): > > > > > > > > > http://codereview.chromium.org/11112021/diff/1/net/http/http_response_body_dr... > > > net/http/http_response_body_drainer_unittest.cc:134: void DelayEofChunk() { > > > delay_eof_chunk_ = true; } > > > Think the logic would be a little simpler if you made this > > > "is_final_chunk_zero_bytes_" and changed behavior accordingly. Then the > > program > > > flow would be a bit more consistent in both cases. > > > > I'm not sure it will be simpler, but more consistent for sure. I'll try to do > > that. > > > > > Note that for unknown reasons, the final chunk is read synchronously, so > you'd > > > currently need to put the code in the fall through case (where "return > > buf_len;" > > > is). > > > > I guess in this case my suspicions are correct and I actually need to remove > the > > unused code on the async pass (num_chunks_ never actually becomes 0 in there). > > But maybe it's better to make consistent and make all chunks assynchronous? > > My thought was to add an is_sync_ bool and have it default to false. And then > add in a test or two that are sync - and to make the existing tests completely > async. If you want, you can go ahead do that in this CL. Or can do it in a followup CL. Either's fine. Or can not do it, and I'll do it myself. Whatever floats your boat. :)
On 2012/10/12 19:05:44, Matt Menke wrote: > > > But maybe it's better to make consistent and make all chunks assynchronous? > > > > My thought was to add an is_sync_ bool and have it default to false. And then > > add in a test or two that are sync - and to make the existing tests completely > > async. If you want, you can go ahead do that in this CL. > > Or can do it in a followup CL. Either's fine. Or can not do it, and I'll do it > myself. Whatever floats your boat. :) I think I'll better do it here. :)
mmenke: please take a look at the modified patch.
This is looking very good, just a couple nits. http://codereview.chromium.org/11112021/diff/10001/net/http/http_response_bod... File net/http/http_response_body_drainer_unittest.cc (right): http://codereview.chromium.org/11112021/diff/10001/net/http/http_response_bod... net/http/http_response_body_drainer_unittest.cc:136: void SetAsync() { is_async_ = true; } I suggest making async the default case, and replacing this with set_sync(). In general, reads will be async, so most tests will then check the more common path. http://codereview.chromium.org/11112021/diff/10001/net/http/http_response_bod... net/http/http_response_body_drainer_unittest.cc:138: void LastChunkZeroSize() { is_last_chunk_zero_size_ = true; } nit: These should actually be set_async() and set_is_last_chunk_zero_size(). (StallReadsForever should also be set_stall_reads_forever()) http://codereview.chromium.org/11112021/diff/10001/net/http/http_response_bod... net/http/http_response_body_drainer_unittest.cc:168: if (num_chunks_ == 0 && !is_last_chunk_zero_size_) I think that rather than handling this here, it should be handled in ReadImpl. If |is_last_chunk_zero_size_| and num_chunks_ is 1, then make the last chunk 0 in the same place you set is_complete_ to true. Makes ReadImpl a little simpler, and reduces redundant code in the is_last_chunk_zero_size_ and non-is_last_chunk_zero_size_ cases. http://codereview.chromium.org/11112021/diff/10001/net/http/http_response_bod... net/http/http_response_body_drainer_unittest.cc:186: void MockHttpStream::ReadImpl() { Think ReadInternal would be a little more consistent. http://codereview.chromium.org/11112021/diff/10001/net/http/http_response_bod... net/http/http_response_body_drainer_unittest.cc:195: else { nit: This should be "} else {" http://codereview.chromium.org/11112021/diff/10001/net/http/http_response_bod... net/http/http_response_body_drainer_unittest.cc:200: callback_.Reset(); Think it's a little weird to put these here. Suggest moving it back to CompleteRead, and only setting it in the async case. http://codereview.chromium.org/11112021/diff/10001/net/http/http_response_bod... net/http/http_response_body_drainer_unittest.cc:257: TEST_F(HttpResponseBodyDrainerTest, DrainBodyAsyncDelayedOK) { How about a comment just before the test along the lines of: "Test the case that the final read chunk is 0 bytes. This can happen when reading the final 0-byte chunk of a chunk encoded file." May also want to rename this test "DrainBodyAsyncEmptyChunk" or some such (And the same for the next one).
On 2012/10/16 14:41:28, Matt Menke wrote: > http://codereview.chromium.org/11112021/diff/10001/net/http/http_response_bod... > File net/http/http_response_body_drainer_unittest.cc (right): > > http://codereview.chromium.org/11112021/diff/10001/net/http/http_response_bod... > net/http/http_response_body_drainer_unittest.cc:136: void SetAsync() { is_async_ > = true; } > I suggest making async the default case, and replacing this with set_sync(). In > general, reads will be async, so most tests will then check the more common > path. Agreed and changed. > http://codereview.chromium.org/11112021/diff/10001/net/http/http_response_bod... > net/http/http_response_body_drainer_unittest.cc:168: if (num_chunks_ == 0 && > !is_last_chunk_zero_size_) > I think that rather than handling this here, it should be handled in ReadImpl. > If |is_last_chunk_zero_size_| and num_chunks_ is 1, then make the last chunk 0 > in the same place you set is_complete_ to true. Makes ReadImpl a little > simpler, and reduces redundant code in the is_last_chunk_zero_size_ and > non-is_last_chunk_zero_size_ cases. I didn't quite understand what you wanted to say here. But I changed this if condition as I didn't like it either. > http://codereview.chromium.org/11112021/diff/10001/net/http/http_response_bod... > net/http/http_response_body_drainer_unittest.cc:186: void > MockHttpStream::ReadImpl() { > Think ReadInternal would be a little more consistent. I don't mind changing the name (although I think "Impl" is closer to the purpose of the method). But could you say it's "more consistent" with what? > http://codereview.chromium.org/11112021/diff/10001/net/http/http_response_bod... > net/http/http_response_body_drainer_unittest.cc:195: else { > nit: This should be "} else {" Grrr... One of the few things I don't like in Google style guide and often forget to comply... :) > http://codereview.chromium.org/11112021/diff/10001/net/http/http_response_bod... > net/http/http_response_body_drainer_unittest.cc:200: callback_.Reset(); > Think it's a little weird to put these here. Suggest moving it back to > CompleteRead, and only setting it in the async case. Done. > http://codereview.chromium.org/11112021/diff/10001/net/http/http_response_bod... > net/http/http_response_body_drainer_unittest.cc:257: > TEST_F(HttpResponseBodyDrainerTest, DrainBodyAsyncDelayedOK) { > How about a comment just before the test along the lines of: > > "Test the case that the final read chunk is 0 bytes. This can happen when > reading the final 0-byte chunk of a chunk encoded file." > > May also want to rename this test "DrainBodyAsyncEmptyChunk" or some such (And > the same for the next one). I rephrased a little, but done.
On 2012/10/16 15:59:50, pivanof wrote: > On 2012/10/16 14:41:28, Matt Menke wrote: > > > http://codereview.chromium.org/11112021/diff/10001/net/http/http_response_bod... > > File net/http/http_response_body_drainer_unittest.cc (right): > > > > > http://codereview.chromium.org/11112021/diff/10001/net/http/http_response_bod... > > net/http/http_response_body_drainer_unittest.cc:136: void SetAsync() { > is_async_ > > = true; } > > I suggest making async the default case, and replacing this with set_sync(). > In > > general, reads will be async, so most tests will then check the more common > > path. > > Agreed and changed. > > > > http://codereview.chromium.org/11112021/diff/10001/net/http/http_response_bod... > > net/http/http_response_body_drainer_unittest.cc:168: if (num_chunks_ == 0 && > > !is_last_chunk_zero_size_) > > I think that rather than handling this here, it should be handled in ReadImpl. > > > If |is_last_chunk_zero_size_| and num_chunks_ is 1, then make the last chunk 0 > > in the same place you set is_complete_ to true. Makes ReadImpl a little > > simpler, and reduces redundant code in the is_last_chunk_zero_size_ and > > non-is_last_chunk_zero_size_ cases. > > I didn't quite understand what you wanted to say here. But I changed this if > condition as I didn't like it either. > > > > http://codereview.chromium.org/11112021/diff/10001/net/http/http_response_bod... > > net/http/http_response_body_drainer_unittest.cc:186: void > > MockHttpStream::ReadImpl() { > > Think ReadInternal would be a little more consistent. > > I don't mind changing the name (although I think "Impl" is closer to the purpose > of the method). But could you say it's "more consistent" with what? Prevailing style. "Impl" is generally used on functions that are internal implementations of the function mae without the impl. Actually, "ReadResponseBodyImpl" would be fine, if you prefer.
http://codereview.chromium.org/11112021/diff/15001/net/http/http_response_bod... File net/http/http_response_body_drainer_unittest.cc (right): http://codereview.chromium.org/11112021/diff/15001/net/http/http_response_bod... net/http/http_response_body_drainer_unittest.cc:186: if (num_chunks_ != 0) { I suggest you put the following here: if (is_last_chunk_zero_size_ && num_chunks_ == 1) buf_len = 0; Then you can remove the "if (num_chunks_ != 0) {", the else branch, and the " && !is_last_chunk_zero_size_"
On 2012/10/16 16:29:22, Matt Menke wrote: > On 2012/10/16 15:59:50, pivanof wrote: > > On 2012/10/16 14:41:28, Matt Menke wrote: http://codereview.chromium.org/11112021/diff/10001/net/http/http_response_bod... > > > net/http/http_response_body_drainer_unittest.cc:186: void > > > MockHttpStream::ReadImpl() { > > > Think ReadInternal would be a little more consistent. > > > > I don't mind changing the name (although I think "Impl" is closer to the > purpose > > of the method). But could you say it's "more consistent" with what? > > Prevailing style. "Impl" is generally used on functions that are internal > implementations of the function mae without the impl. Actually, > "ReadResponseBodyImpl" would be fine, if you prefer. Err... mae = name.
On 2012/10/16 16:32:24, Matt Menke wrote: > http://codereview.chromium.org/11112021/diff/15001/net/http/http_response_bod... > File net/http/http_response_body_drainer_unittest.cc (right): > > http://codereview.chromium.org/11112021/diff/15001/net/http/http_response_bod... > net/http/http_response_body_drainer_unittest.cc:186: if (num_chunks_ != 0) { > I suggest you put the following here: > > if (is_last_chunk_zero_size_ && num_chunks_ == 1) > buf_len = 0; > > Then you can remove the "if (num_chunks_ != 0) {", the else branch, and the " && > !is_last_chunk_zero_size_" OH, and to clarify - then the zero byte chunk would be included in num_chunks_, rather than be an extra bonus chunk.
Now I understand what you tried to say all along. That's a good cleaning of the code, I've updated it.
On 2012/10/17 05:29:11, pivanof wrote: > Now I understand what you tried to say all along. That's a good cleaning of the > code, I've updated it. LGTM. Thanks for the fix and the cleanup. Land whenever you like.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/paivanof@gmail.com/11112021/23001
Change committed as 162415 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
