Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
920483004:
Added chunked upload support for embedded_test_server (Closed)
DescriptionAdded chunked upload support for embedded_test_server
This CL adds chunked upload support for embedded test server, so that Cronet
can test chunked upload using this test server.
BUG=412942
Committed: https://crrev.com/cd060ba2b176cc322eba5ed552cdf4506d22efd2
Cr-Commit-Position: refs/heads/master@{#316271}
Patch Set 1 : Self review #
Total comments: 12
Patch Set 2 : #
Total comments: 6
Patch Set 3 : Matt's comments #
Total comments: 10
Patch Set 4 : Addressed new comments #Patch Set 5 : Modified test #
Messages
Total messages: 21 (5 generated)
xunjieli@chromium.org changed reviewers: + mmenke@chromium.org
Patchset #1 (id:1) has been deleted
That was fast. https://codereview.chromium.org/920483004/diff/20001/net/test/embedded_test_s... File net/test/embedded_test_server/http_request.cc (right): https://codereview.chromium.org/920483004/diff/20001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.cc:137: if (http_request_->headers.count("Transfer-Encoding") > 0) { Suggest flipping the order - ignoring a Content-Length tag when there's a tranfer-encoding other than chunked seems a little weird (Admittedly, the server isn't doing what it should in that case, anyways) https://codereview.chromium.org/920483004/diff/20001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.cc:165: if (chunked_decoder_.get()) { nit: The ".get()" isn't needed https://codereview.chromium.org/920483004/diff/20001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.cc:169: &result[0], static_cast<int>(result.size())); Why do we need result? Can't we just use: bytes_written = chunked_decoder_->FilterBuf(buffer_.data() + buffer_position_, available_bytes);? http_request_->content.append(buffer_.data() + buffer_position_, bytes_written); buffer_position_ += bytes_written; (Note that this is odd code - we can either not apparent each read until the end, or we can just always read from the start of the buffer, but fixing that is beyond the scope of this CL) https://codereview.chromium.org/920483004/diff/20001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.cc:171: buffer_position_ += bytes_written; This should be outside of the if block. The fact tests didn't catch that means we should probably have another test - slowly append upload data on a timer? Normally not a big fan of timers, but appending a 1 character chunk ever 10 milliseconds a couple times doesn't seem to bad. https://codereview.chromium.org/920483004/diff/20001/net/test/embedded_test_s... File net/test/embedded_test_server/http_request.h (right): https://codereview.chromium.org/920483004/diff/20001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.h:14: #include "net/http/http_chunked_decoder.h" Should forward declare this and include the header in the cc file
Two unresolved comments below. PTAL. Thanks! https://codereview.chromium.org/920483004/diff/20001/net/test/embedded_test_s... File net/test/embedded_test_server/http_request.cc (right): https://codereview.chromium.org/920483004/diff/20001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.cc:137: if (http_request_->headers.count("Transfer-Encoding") > 0) { On 2015/02/12 15:34:30, mmenke wrote: > Suggest flipping the order - ignoring a Content-Length tag when there's a > tranfer-encoding other than chunked seems a little weird (Admittedly, the server > isn't doing what it should in that case, anyways) Done. https://codereview.chromium.org/920483004/diff/20001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.cc:165: if (chunked_decoder_.get()) { On 2015/02/12 15:34:30, mmenke wrote: > nit: The ".get()" isn't needed Done. https://codereview.chromium.org/920483004/diff/20001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.cc:169: &result[0], static_cast<int>(result.size())); On 2015/02/12 15:34:30, mmenke wrote: > Why do we need result? Can't we just use: > > bytes_written = chunked_decoder_->FilterBuf(buffer_.data() + buffer_position_, > available_bytes);? > http_request_->content.append(buffer_.data() + buffer_position_, bytes_written); > buffer_position_ += bytes_written; > > (Note that this is odd code - we can either not apparent each read until the > end, or we can just always read from the start of the buffer, but fixing that is > beyond the scope of this CL) When I tried "chunked_decoder_->FilterBuf(buffer_.data() + buffer_position_, available_bytes)", it complains that buffer_.data() + buffer_position_ is a const char*, but it expects a char*. I tried to do "buffer_position += bytes_written" every time, but it seems like FilterBuf can only parse data if the data is prefixed with size, so we have to do FilterBuf from the start of the buffer.. https://codereview.chromium.org/920483004/diff/20001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.cc:171: buffer_position_ += bytes_written; On 2015/02/12 15:34:30, mmenke wrote: > This should be outside of the if block. The fact tests didn't catch that means > we should probably have another test - slowly append upload data on a timer? > Normally not a big fan of timers, but appending a 1 character chunk ever 10 > milliseconds a couple times doesn't seem to bad. It looks like FilterBuf can only parse chunked data if it is prefixed with chunked size, so we need to parse from the start of the buffer. Therefore, I incremented buffer_position, only when we reached eof. https://codereview.chromium.org/920483004/diff/20001/net/test/embedded_test_s... File net/test/embedded_test_server/http_request.h (right): https://codereview.chromium.org/920483004/diff/20001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.h:14: #include "net/http/http_chunked_decoder.h" On 2015/02/12 15:34:30, mmenke wrote: > Should forward declare this and include the header in the cc file Done.
https://codereview.chromium.org/920483004/diff/20001/net/test/embedded_test_s... File net/test/embedded_test_server/http_request.cc (right): https://codereview.chromium.org/920483004/diff/20001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.cc:169: &result[0], static_cast<int>(result.size())); On 2015/02/12 15:58:40, xunjieli wrote: > On 2015/02/12 15:34:30, mmenke wrote: > > Why do we need result? Can't we just use: > > > > bytes_written = chunked_decoder_->FilterBuf(buffer_.data() + buffer_position_, > > available_bytes);? > > http_request_->content.append(buffer_.data() + buffer_position_, > bytes_written); > > buffer_position_ += bytes_written; > > > > (Note that this is odd code - we can either not apparent each read until the > > end, or we can just always read from the start of the buffer, but fixing that > is > > beyond the scope of this CL) > > When I tried "chunked_decoder_->FilterBuf(buffer_.data() + buffer_position_, > available_bytes)", it complains that buffer_.data() + buffer_position_ is a > const char*, but it expects a char*. > > I tried to do "buffer_position += bytes_written" every time, but it seems like > FilterBuf can only parse data if the data is prefixed with size, so we have to > do FilterBuf from the start of the buffer.. That's because buffer_ is a string (I had assumed it was an io_buffer_). I suggest switching it to a std::vector<char>, though I haven't looked into all the implications of doing so. https://codereview.chromium.org/920483004/diff/20001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.cc:171: buffer_position_ += bytes_written; On 2015/02/12 15:58:40, xunjieli wrote: > On 2015/02/12 15:34:30, mmenke wrote: > > This should be outside of the if block. The fact tests didn't catch that > means > > we should probably have another test - slowly append upload data on a timer? > > Normally not a big fan of timers, but appending a 1 character chunk ever 10 > > milliseconds a couple times doesn't seem to bad. > > It looks like FilterBuf can only parse chunked data if it is prefixed with > chunked size, so we need to parse from the start of the buffer. Therefore, I > incremented buffer_position, only when we reached eof. You're misreading the code. FilterBuf is a streaming function, and buffers data if it doesn't have a complete chunked. If you call it on a partial chunk header...FilterBuf("100"), it will fill the buffer with the partial data...And then if you call it again with "100\r\nBlah", it will incorrectly interpret that data as the continuation of the partial chunk you already passed it - i.e., it will think you've received "100100\r\nBlah". Note that once it's parsed the chunk length, it will just stream the data to you (in the earlier case, you'd call FilterBuf("100"), it would read nothing, then you'd call FilterBuf("\r\nBlah"), and it would set the buffer to "Blah" and return 4, so it never needs more space than the number of bytes you're passing in to it) It's meant to be used incrementally...If we're receiving a 15 GB chunked download, we can't cache the entire file in memory.
Thanks for the review! https://codereview.chromium.org/920483004/diff/40001/net/test/embedded_test_s... File net/test/embedded_test_server/http_request.cc (right): https://codereview.chromium.org/920483004/diff/40001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.cc:171: buffer_position_ += available_bytes; I know why I was having issue with FilterBuf. we should not increment buffer_position by the bytes_written, which is the number of bytes written after eliminating CRLF. But I am not sure if we should increment available_bytes either, since there might be a case whether FilterBuf does not parse everything available right? What do you suggest? Changing buffer_ to std::vector<char> affects many places in this file (since the parser uses find() method on string), so I think I will keep std::string result to store the intermediate.
https://codereview.chromium.org/920483004/diff/40001/net/test/embedded_test_s... File net/test/embedded_test_server/http_request.cc (right): https://codereview.chromium.org/920483004/diff/40001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.cc:170: &result[0], static_cast<int>(result.size())); &result[0] -> result.data() https://codereview.chromium.org/920483004/diff/40001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.cc:171: buffer_position_ += available_bytes; On 2015/02/12 16:54:52, xunjieli wrote: > I know why I was having issue with FilterBuf. we should not increment > buffer_position by the bytes_written, which is the number of bytes written after > eliminating CRLF. > > But I am not sure if we should increment available_bytes either, since there > might be a case whether FilterBuf does not parse everything available right? FilterBuf will parse everything you give it, up to the trailer, per my earlier comment. > What do you suggest? Clearing buffer_ and resetting buffer_position_ after each pass, except the last, where you should keep the last bytes_after_eof bytes (And also reset buffer_position_). Also, as you're getting nothing out of using result, which is a non-const string, instead of buffer_, which is a non-const string...get rid of it. > Changing buffer_ to std::vector<char> affects many places in this file (since > the parser uses find() method on string), so I think I will keep std::string > result to store the intermediate. https://codereview.chromium.org/920483004/diff/40001/net/test/embedded_test_s... File net/test/embedded_test_server/http_request_unittest.cc (right): https://codereview.chromium.org/920483004/diff/40001/net/test/embedded_test_s... net/test/embedded_test_server/http_request_unittest.cc:95: } Another suggested test: std::string data = "POST /foobar.html HTTP/1.1\r\n" "Transfer-Encoding: chunked\r\n\r\n" "5\r\nhello\r\n" ... for (int i = 0; i < data.length; ++i) { parser.ProcessChunk(data[i]); } This will hit the problem with your code.
Patchset #3 (id:60001) has been deleted
Uploaded a new patch. PTAL. Thanks! https://codereview.chromium.org/920483004/diff/40001/net/test/embedded_test_s... File net/test/embedded_test_server/http_request.cc (right): https://codereview.chromium.org/920483004/diff/40001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.cc:171: buffer_position_ += available_bytes; I guess one benefit of using result is that we won't be change buffer_ . https://codereview.chromium.org/920483004/diff/80001/net/test/embedded_test_s... File net/test/embedded_test_server/http_request.cc (right): https://codereview.chromium.org/920483004/diff/80001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.cc:167: int bytes_written = chunked_decoder_->FilterBuf(&buffer_[buffer_position_], I can't use buffer_.data() here, as it complain that it is a const char *. What else can I try? (besides changing buffer_ to vector<char>)
Sorry for the spam. Forgot to upload this comment. PTAL. thanks! https://codereview.chromium.org/920483004/diff/40001/net/test/embedded_test_s... File net/test/embedded_test_server/http_request_unittest.cc (right): https://codereview.chromium.org/920483004/diff/40001/net/test/embedded_test_s... net/test/embedded_test_server/http_request_unittest.cc:95: } On 2015/02/12 18:22:48, mmenke wrote: > Another suggested test: > > std::string data = > "POST /foobar.html HTTP/1.1\r\n" > "Transfer-Encoding: chunked\r\n\r\n" > "5\r\nhello\r\n" > ... > > for (int i = 0; i < data.length; ++i) { > parser.ProcessChunk(data[i]); > } > > This will hit the problem with your code. Done.
https://codereview.chromium.org/920483004/diff/80001/net/test/embedded_test_s... File net/test/embedded_test_server/http_request.cc (right): https://codereview.chromium.org/920483004/diff/80001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.cc:167: int bytes_written = chunked_decoder_->FilterBuf(&buffer_[buffer_position_], On 2015/02/12 22:15:45, xunjieli wrote: > I can't use buffer_.data() here, as it complain that it is a const char *. What > else can I try? (besides changing buffer_ to vector<char>) Looks like in StringIOBuffer we use const_cast<char*>(buffer_.data()). Suggest using that here, and then you can add buffer_position_ to that. Not a fan of that, either, but at least we're already depending on it working. I would not be surprised if we're depending on this pattern working elsewhere, but I'm a bit happier using data(). https://codereview.chromium.org/920483004/diff/80001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.cc:175: chunked_decoder_->bytes_after_eof()); Suggest buffer_ = buffer_.substring(buffer_.size() - chunked_decoder_->bytes_after_eof()); (Or could use chunked_decoder_->bytes_after_eof() as the second parameter) https://codereview.chromium.org/920483004/diff/80001/net/test/embedded_test_s... File net/test/embedded_test_server/http_request_unittest.cc (right): https://codereview.chromium.org/920483004/diff/80001/net/test/embedded_test_s... net/test/embedded_test_server/http_request_unittest.cc:90: scoped_ptr<HttpRequest> request = parser.GetRequest(); Should include base/memory/scoped_ptr (File already depends on it, but it's not explicitly being included) https://codereview.chromium.org/920483004/diff/80001/net/test/embedded_test_s... net/test/embedded_test_server/http_request_unittest.cc:104: parser.ProcessChunk("0\r\n\r\n"); You need to send partial chunks here, not entire chunks at once. That was the whole point processing the response character-by-character.
Thanks for the review! PTAL. https://codereview.chromium.org/920483004/diff/80001/net/test/embedded_test_s... File net/test/embedded_test_server/http_request.cc (right): https://codereview.chromium.org/920483004/diff/80001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.cc:167: int bytes_written = chunked_decoder_->FilterBuf(&buffer_[buffer_position_], On 2015/02/13 17:53:29, mmenke wrote: > On 2015/02/12 22:15:45, xunjieli wrote: > > I can't use buffer_.data() here, as it complain that it is a const char *. > What > > else can I try? (besides changing buffer_ to vector<char>) > > Looks like in StringIOBuffer we use const_cast<char*>(buffer_.data()). Suggest > using that here, and then you can add buffer_position_ to that. Not a fan of > that, either, but at least we're already depending on it working. I would not > be surprised if we're depending on this pattern working elsewhere, but I'm a bit > happier using data(). Done. SGTM! casting away constness it is! https://codereview.chromium.org/920483004/diff/80001/net/test/embedded_test_s... net/test/embedded_test_server/http_request.cc:175: chunked_decoder_->bytes_after_eof()); On 2015/02/13 17:53:29, mmenke wrote: > Suggest buffer_ = buffer_.substring(buffer_.size() - > chunked_decoder_->bytes_after_eof()); (Or could use > chunked_decoder_->bytes_after_eof() as the second parameter) Done. https://codereview.chromium.org/920483004/diff/80001/net/test/embedded_test_s... File net/test/embedded_test_server/http_request_unittest.cc (right): https://codereview.chromium.org/920483004/diff/80001/net/test/embedded_test_s... net/test/embedded_test_server/http_request_unittest.cc:90: scoped_ptr<HttpRequest> request = parser.GetRequest(); On 2015/02/13 17:53:29, mmenke wrote: > Should include base/memory/scoped_ptr (File already depends on it, but it's not > explicitly being included) Done. https://codereview.chromium.org/920483004/diff/80001/net/test/embedded_test_s... net/test/embedded_test_server/http_request_unittest.cc:104: parser.ProcessChunk("0\r\n\r\n"); On 2015/02/13 17:53:29, mmenke wrote: > You need to send partial chunks here, not entire chunks at once. That was the > whole point processing the response character-by-character. Right, that's why I called parser.ParseRequest() on line 103, and again on line 105. Should I stop at line 104 then?
https://codereview.chromium.org/920483004/diff/80001/net/test/embedded_test_s... File net/test/embedded_test_server/http_request_unittest.cc (right): https://codereview.chromium.org/920483004/diff/80001/net/test/embedded_test_s... net/test/embedded_test_server/http_request_unittest.cc:104: parser.ProcessChunk("0\r\n\r\n"); On 2015/02/13 18:36:14, xunjieli wrote: > On 2015/02/13 17:53:29, mmenke wrote: > > You need to send partial chunks here, not entire chunks at once. That was the > > whole point processing the response character-by-character. > > Right, that's why I called parser.ParseRequest() on line 103, and again on line > 105. Should I stop at line 104 then? You're still calling it at chunk boundaries...So if you're using the chunked decoder incorrectly, but in a way that works as long as you only call it at chunk boundaries, this test would incorrectly pass. Should append data one character at a time and then call ParseRequest, to make sure sure everything works (In my earlier example, hadn't realized ProcessChunk doesn't call into ParseRequest)
Patchset #5 (id:120001) has been deleted
> You're still calling it at chunk boundaries...So if you're using the chunked > decoder incorrectly, but in a way that works as long as you only call it at > chunk boundaries, this test would incorrectly pass. Should append data one > character at a time and then call ParseRequest, to make sure sure everything > works (In my earlier example, hadn't realized ProcessChunk doesn't call into > ParseRequest) I finally got it! Thanks so much for taking the time to explain! I have uploaded a new patch where I sent one character at a time, which should test the chunk boundaries scenario that you've mentioned. Thanks! PTAL
On 2015/02/13 19:12:09, xunjieli wrote: > > You're still calling it at chunk boundaries...So if you're using the chunked > > decoder incorrectly, but in a way that works as long as you only call it at > > chunk boundaries, this test would incorrectly pass. Should append data one > > character at a time and then call ParseRequest, to make sure sure everything > > works (In my earlier example, hadn't realized ProcessChunk doesn't call into > > ParseRequest) > > I finally got it! Thanks so much for taking the time to explain! I have uploaded > a new patch where I sent one character at a time, which should test the chunk > boundaries scenario that you've mentioned. Thanks! PTAL LGTM
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/920483004/140001
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/cd060ba2b176cc322eba5ed552cdf4506d22efd2 Cr-Commit-Position: refs/heads/master@{#316271} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
