Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
212683006:
HttpServer: allows sending raw response data for nontypical responses. (Closed)
|
Created:
6 years, 9 months ago by gunsch Modified:
6 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionHttpServer: allows sending raw response data for nontypical responses.
BUG=347770
R=rsleevi@chromium.org,lcwu@chromium.org,byungchul@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263948
Patch Set 1 #Patch Set 2 : Brief docs #Patch Set 3 : Unit tests involving response-checking. #
Total comments: 11
Patch Set 4 : Review comments, even more tests. #Patch Set 5 : Replaces request+connection with std::pair. #
Total comments: 8
Patch Set 6 : rsleevi review fixes #
Total comments: 1
Patch Set 7 : Address rsleevi comments #
Total comments: 6
Patch Set 8 : Unit test robustness #
Messages
Total messages: 26 (0 generated)
There need to be tests. Both your CL and your bug do not explain what "non-typical" data is, nor does the documentation for the function provide a clear guidance for when and how to use this. I'd like to better understand a) The problem b) How the solution came to be (eg: what alternatives were considered/discarded) c) How to use this correctly/safely d) When to use this
On 2014/03/26 19:10:38, Ryan Sleevi wrote: > There need to be tests. > > Both your CL and your bug do not explain what "non-typical" data is, nor does > the documentation for the function provide a clear guidance for when and how to > use this. > > I'd like to better understand > a) The problem > b) How the solution came to be (eg: what alternatives were considered/discarded) > c) How to use this correctly/safely > d) When to use this On 2014/03/26 19:10:38, Ryan Sleevi wrote: > There need to be tests. > > Both your CL and your bug do not explain what "non-typical" data is, nor does > the documentation for the function provide a clear guidance for when and how to > use this. > > I'd like to better understand > a) The problem > b) How the solution came to be (eg: what alternatives were considered/discarded) > c) How to use this correctly/safely > d) When to use this I'll quote what I had added to the bug, to have this in one place: "One of Chromecast's use cases is to allow downloading large files (such as the system log), which it sometimes sends in streamed or chunked manners." There are a few handlers where we are serving large files: One call site in our existing code serves files from disk in a streamed way (reading from disk and sending chunks of data). Another sends down a series of logs and state dumps with "Transfer-Encoding: chunked". As part of the effort to get Chromecast's codebase back onto Chromium and off of our local fork, this is one of the pieces that is depended on. However, since I joined the team well past the decision-making point for the existing code, I'll have to defer to Byungchul or Le-Chun (both included here) to explain. I'll look into adding tests while we continue this discussion. I was surprised that HttpServerUnitTest doesn't seem to test any of the Send methods.
On 2014/03/26 19:40:24, gunsch wrote: > On 2014/03/26 19:10:38, Ryan Sleevi wrote: > > There need to be tests. > > > > Both your CL and your bug do not explain what "non-typical" data is, nor does > > the documentation for the function provide a clear guidance for when and how > to > > use this. > > > > I'd like to better understand > > a) The problem > > b) How the solution came to be (eg: what alternatives were > considered/discarded) > > c) How to use this correctly/safely > > d) When to use this > > On 2014/03/26 19:10:38, Ryan Sleevi wrote: > > There need to be tests. > > > > Both your CL and your bug do not explain what "non-typical" data is, nor does > > the documentation for the function provide a clear guidance for when and how > to > > use this. > > > > I'd like to better understand > > a) The problem > > b) How the solution came to be (eg: what alternatives were > considered/discarded) > > c) How to use this correctly/safely > > d) When to use this > > I'll quote what I had added to the bug, to have this in one place: > > "One of Chromecast's use cases is to allow downloading large files (such as the > system log), which it sometimes sends in streamed or chunked manners." > > There are a few handlers where we are serving large files: > One call site in our existing code serves files from disk in a streamed way > (reading from disk and sending chunks of data). > Another sends down a series of logs and state dumps with "Transfer-Encoding: > chunked". > > As part of the effort to get Chromecast's codebase back onto Chromium and off of > our local fork, this is one of the pieces that is depended on. However, since I > joined the team well past the decision-making point for the existing code, I'll > have to defer to Byungchul or Le-Chun (both included here) to explain. > > I'll look into adding tests while we continue this discussion. I was surprised > that HttpServerUnitTest doesn't seem to test any of the Send methods. Added a test for SendRaw, as well as one for Send200. This involved instrumenting HttpServerTest's TestHttpClient with response-reading code.
https://codereview.chromium.org/212683006/diff/40001/net/server/http_server_u... File net/server/http_server_unittest.cc (right): https://codereview.chromium.org/212683006/diff/40001/net/server/http_server_u... net/server/http_server_unittest.cc:41: int kMaxExpectedResponseLength = 2048; const https://codereview.chromium.org/212683006/diff/40001/net/server/http_server_u... net/server/http_server_unittest.cc:124: int result = socket_->Read(read_buffer_, kMaxExpectedResponseLength, nit: I believe kMaxExpectedResponseLength goes on separate line. https://codereview.chromium.org/212683006/diff/40001/net/server/http_server_u... net/server/http_server_unittest.cc:176: virtual void OnClose(int connection_id) OVERRIDE {} Should it remove the |connection_id| from |connection_ids_|? https://codereview.chromium.org/212683006/diff/40001/net/server/http_server_u... net/server/http_server_unittest.cc:318: ASSERT_TRUE(client.Read(&response)); Would it make sense to also test a scenario where client reads are mixed with server sends?
https://codereview.chromium.org/212683006/diff/40001/net/server/http_server_u... File net/server/http_server_unittest.cc (right): https://codereview.chromium.org/212683006/diff/40001/net/server/http_server_u... net/server/http_server_unittest.cc:41: int kMaxExpectedResponseLength = 2048; On 2014/03/27 15:03:45, mef wrote: > const Done. https://codereview.chromium.org/212683006/diff/40001/net/server/http_server_u... net/server/http_server_unittest.cc:124: int result = socket_->Read(read_buffer_, kMaxExpectedResponseLength, On 2014/03/27 15:03:45, mef wrote: > nit: I believe kMaxExpectedResponseLength goes on separate line. Done. https://codereview.chromium.org/212683006/diff/40001/net/server/http_server_u... net/server/http_server_unittest.cc:176: virtual void OnClose(int connection_id) OVERRIDE {} On 2014/03/27 15:03:45, mef wrote: > Should it remove the |connection_id| from |connection_ids_|? I lean towards _no_, to keep |requests_| and |connection_ids_| in sync. I feel like it might be more appropriate to add a small struct for (request, connection_id) pairs and only keep a vector of those structs. That's my intuition coming more from Java though, would that be appropriate here as well? https://codereview.chromium.org/212683006/diff/40001/net/server/http_server_u... net/server/http_server_unittest.cc:318: ASSERT_TRUE(client.Read(&response)); On 2014/03/27 15:03:45, mef wrote: > Would it make sense to also test a scenario where client reads are mixed with > server sends? Sure, I extended the MultipleRequestsOnSameConnection test to send and verify several responses on the same connection.
https://codereview.chromium.org/212683006/diff/40001/net/server/http_server_u... File net/server/http_server_unittest.cc (right): https://codereview.chromium.org/212683006/diff/40001/net/server/http_server_u... net/server/http_server_unittest.cc:318: ASSERT_TRUE(client.Read(&response)); On 2014/03/27 15:52:39, gunsch wrote: > On 2014/03/27 15:03:45, mef wrote: > > Would it make sense to also test a scenario where client reads are mixed with > > server sends? > > Sure, I extended the MultipleRequestsOnSameConnection test to send and verify > several responses on the same connection. I just went ahead and made this a std::pair. I think that's clearer.
https://codereview.chromium.org/212683006/diff/40001/net/server/http_server_u... File net/server/http_server_unittest.cc (right): https://codereview.chromium.org/212683006/diff/40001/net/server/http_server_u... net/server/http_server_unittest.cc:137: base::Closure run_loop_quit_func_; You shouldn't need this separate RunLoop helper. You should be able to use TestCompletionCallback w/ WaitForResult() to handle just this. https://codereview.chromium.org/212683006/diff/80001/net/server/http_server_u... File net/server/http_server_unittest.cc (right): https://codereview.chromium.org/212683006/diff/80001/net/server/http_server_u... net/server/http_server_unittest.cc:123: void Read(const base::Closure& quit_loop) { This overloads with the Read method on 92. Rename this something like ReadInternal? https://codereview.chromium.org/212683006/diff/80001/net/server/http_server_u... net/server/http_server_unittest.cc:191: HttpServerRequestInfo request(int request_index) { naming: GetRequest() defensive: Should you DCHECK_LE(request_index, requests_.size()) type safety: size_t request_index, not int. https://codereview.chromium.org/212683006/diff/80001/net/server/http_server_u... net/server/http_server_unittest.cc:195: int connection_id(int request_index) { naming: GetConnectionId() defensive: Should you DCHECK_LE(request_index, requests_.size()) type safety: size_t request_index, not int. https://codereview.chromium.org/212683006/diff/80001/net/server/http_server_u... net/server/http_server_unittest.cc:313: ASSERT_TRUE(response.find("Response!") != std::string::npos); ASSERT_NE(std::string::npos, response.find("Response!")) ?
https://codereview.chromium.org/212683006/diff/40001/net/server/http_server_u... File net/server/http_server_unittest.cc (right): https://codereview.chromium.org/212683006/diff/40001/net/server/http_server_u... net/server/http_server_unittest.cc:137: base::Closure run_loop_quit_func_; On 2014/03/27 20:59:46, Ryan Sleevi wrote: > You shouldn't need this separate RunLoop helper. You should be able to use > TestCompletionCallback w/ WaitForResult() to handle just this. Done, thanks for the tip. That's a lot simpler. https://codereview.chromium.org/212683006/diff/80001/net/server/http_server_u... File net/server/http_server_unittest.cc (right): https://codereview.chromium.org/212683006/diff/80001/net/server/http_server_u... net/server/http_server_unittest.cc:123: void Read(const base::Closure& quit_loop) { On 2014/03/27 20:59:46, Ryan Sleevi wrote: > This overloads with the Read method on 92. Rename this something like > ReadInternal? Done. https://codereview.chromium.org/212683006/diff/80001/net/server/http_server_u... net/server/http_server_unittest.cc:191: HttpServerRequestInfo request(int request_index) { On 2014/03/27 20:59:46, Ryan Sleevi wrote: > naming: GetRequest() > defensive: Should you DCHECK_LE(request_index, requests_.size()) > type safety: size_t request_index, not int. Done. https://codereview.chromium.org/212683006/diff/80001/net/server/http_server_u... net/server/http_server_unittest.cc:195: int connection_id(int request_index) { On 2014/03/27 20:59:46, Ryan Sleevi wrote: > naming: GetConnectionId() > defensive: Should you DCHECK_LE(request_index, requests_.size()) > type safety: size_t request_index, not int. Done. https://codereview.chromium.org/212683006/diff/80001/net/server/http_server_u... net/server/http_server_unittest.cc:313: ASSERT_TRUE(response.find("Response!") != std::string::npos); On 2014/03/27 20:59:46, Ryan Sleevi wrote: > ASSERT_NE(std::string::npos, response.find("Response!")) ? Done.
On 2014/03/27 23:41:16, gunsch wrote: > https://codereview.chromium.org/212683006/diff/40001/net/server/http_server_u... > File net/server/http_server_unittest.cc (right): > > https://codereview.chromium.org/212683006/diff/40001/net/server/http_server_u... > net/server/http_server_unittest.cc:137: base::Closure run_loop_quit_func_; > On 2014/03/27 20:59:46, Ryan Sleevi wrote: > > You shouldn't need this separate RunLoop helper. You should be able to use > > TestCompletionCallback w/ WaitForResult() to handle just this. > > Done, thanks for the tip. That's a lot simpler. > > https://codereview.chromium.org/212683006/diff/80001/net/server/http_server_u... > File net/server/http_server_unittest.cc (right): > > https://codereview.chromium.org/212683006/diff/80001/net/server/http_server_u... > net/server/http_server_unittest.cc:123: void Read(const base::Closure& > quit_loop) { > On 2014/03/27 20:59:46, Ryan Sleevi wrote: > > This overloads with the Read method on 92. Rename this something like > > ReadInternal? > > Done. > > https://codereview.chromium.org/212683006/diff/80001/net/server/http_server_u... > net/server/http_server_unittest.cc:191: HttpServerRequestInfo request(int > request_index) { > On 2014/03/27 20:59:46, Ryan Sleevi wrote: > > naming: GetRequest() > > defensive: Should you DCHECK_LE(request_index, requests_.size()) > > type safety: size_t request_index, not int. > > Done. > > https://codereview.chromium.org/212683006/diff/80001/net/server/http_server_u... > net/server/http_server_unittest.cc:195: int connection_id(int request_index) { > On 2014/03/27 20:59:46, Ryan Sleevi wrote: > > naming: GetConnectionId() > > defensive: Should you DCHECK_LE(request_index, requests_.size()) > > type safety: size_t request_index, not int. > > Done. > > https://codereview.chromium.org/212683006/diff/80001/net/server/http_server_u... > net/server/http_server_unittest.cc:313: ASSERT_TRUE(response.find("Response!") > != std::string::npos); > On 2014/03/27 20:59:46, Ryan Sleevi wrote: > > ASSERT_NE(std::string::npos, response.find("Response!")) ? > > Done. Ping?
On 2014/04/02 00:57:49, gunsch wrote: > On 2014/03/27 23:41:16, gunsch wrote: > > > https://codereview.chromium.org/212683006/diff/40001/net/server/http_server_u... > > File net/server/http_server_unittest.cc (right): > > > > > https://codereview.chromium.org/212683006/diff/40001/net/server/http_server_u... > > net/server/http_server_unittest.cc:137: base::Closure run_loop_quit_func_; > > On 2014/03/27 20:59:46, Ryan Sleevi wrote: > > > You shouldn't need this separate RunLoop helper. You should be able to use > > > TestCompletionCallback w/ WaitForResult() to handle just this. > > > > Done, thanks for the tip. That's a lot simpler. > > > > > https://codereview.chromium.org/212683006/diff/80001/net/server/http_server_u... > > File net/server/http_server_unittest.cc (right): > > > > > https://codereview.chromium.org/212683006/diff/80001/net/server/http_server_u... > > net/server/http_server_unittest.cc:123: void Read(const base::Closure& > > quit_loop) { > > On 2014/03/27 20:59:46, Ryan Sleevi wrote: > > > This overloads with the Read method on 92. Rename this something like > > > ReadInternal? > > > > Done. > > > > > https://codereview.chromium.org/212683006/diff/80001/net/server/http_server_u... > > net/server/http_server_unittest.cc:191: HttpServerRequestInfo request(int > > request_index) { > > On 2014/03/27 20:59:46, Ryan Sleevi wrote: > > > naming: GetRequest() > > > defensive: Should you DCHECK_LE(request_index, requests_.size()) > > > type safety: size_t request_index, not int. > > > > Done. > > > > > https://codereview.chromium.org/212683006/diff/80001/net/server/http_server_u... > > net/server/http_server_unittest.cc:195: int connection_id(int request_index) { > > On 2014/03/27 20:59:46, Ryan Sleevi wrote: > > > naming: GetConnectionId() > > > defensive: Should you DCHECK_LE(request_index, requests_.size()) > > > type safety: size_t request_index, not int. > > > > Done. > > > > > https://codereview.chromium.org/212683006/diff/80001/net/server/http_server_u... > > net/server/http_server_unittest.cc:313: ASSERT_TRUE(response.find("Response!") > > != std::string::npos); > > On 2014/03/27 20:59:46, Ryan Sleevi wrote: > > > ASSERT_NE(std::string::npos, response.find("Response!")) ? > > > > Done. > > Ping? Sorry for forgetting Jesse's request. This api is necessary as Jesse already mentioned to send back via http protocol 1) /etc/LICENSE.html.gz to show license terms applied in the device which is about 120Kbytes 2) logging messages which could be more than 1Mbytes 1) is inevitable because of open license policy. 2) is not enabled for production, but still useful to debug the device with eng build.
Thank you for explaining, byungchul. I agree that this API probably meets those goals, but my feeling is that it does so fairly uncleanly - exposing the 'raw' Send method is somewhat of an abstraction cop-out. I will take another look at this, but it seems your requirements could also be met if you were allowed to send incremental data, is that correct? That is, you have no need for manually manipulating or constructing the response or headers, you just wish to be able to stream data? Have I missed something in your requirements? On Apr 2, 2014 7:58 AM, <byungchul@chromium.org> wrote: > On 2014/04/02 00:57:49, gunsch wrote: > >> On 2014/03/27 23:41:16, gunsch wrote: >> > >> > > https://codereview.chromium.org/212683006/diff/40001/net/ > server/http_server_unittest.cc > >> > File net/server/http_server_unittest.cc (right): >> > >> > >> > > https://codereview.chromium.org/212683006/diff/40001/net/ > server/http_server_unittest.cc#newcode137 > >> > net/server/http_server_unittest.cc:137: base::Closure >> run_loop_quit_func_; >> > On 2014/03/27 20:59:46, Ryan Sleevi wrote: >> > > You shouldn't need this separate RunLoop helper. You should be able >> to use >> > > TestCompletionCallback w/ WaitForResult() to handle just this. >> > >> > Done, thanks for the tip. That's a lot simpler. >> > >> > >> > > https://codereview.chromium.org/212683006/diff/80001/net/ > server/http_server_unittest.cc > >> > File net/server/http_server_unittest.cc (right): >> > >> > >> > > https://codereview.chromium.org/212683006/diff/80001/net/ > server/http_server_unittest.cc#newcode123 > >> > net/server/http_server_unittest.cc:123: void Read(const base::Closure& >> > quit_loop) { >> > On 2014/03/27 20:59:46, Ryan Sleevi wrote: >> > > This overloads with the Read method on 92. Rename this something like >> > > ReadInternal? >> > >> > Done. >> > >> > >> > > https://codereview.chromium.org/212683006/diff/80001/net/ > server/http_server_unittest.cc#newcode191 > >> > net/server/http_server_unittest.cc:191: HttpServerRequestInfo >> request(int >> > request_index) { >> > On 2014/03/27 20:59:46, Ryan Sleevi wrote: >> > > naming: GetRequest() >> > > defensive: Should you DCHECK_LE(request_index, requests_.size()) >> > > type safety: size_t request_index, not int. >> > >> > Done. >> > >> > >> > > https://codereview.chromium.org/212683006/diff/80001/net/ > server/http_server_unittest.cc#newcode195 > >> > net/server/http_server_unittest.cc:195: int connection_id(int >> request_index) >> > { > >> > On 2014/03/27 20:59:46, Ryan Sleevi wrote: >> > > naming: GetConnectionId() >> > > defensive: Should you DCHECK_LE(request_index, requests_.size()) >> > > type safety: size_t request_index, not int. >> > >> > Done. >> > >> > >> > > https://codereview.chromium.org/212683006/diff/80001/net/ > server/http_server_unittest.cc#newcode313 > >> > net/server/http_server_unittest.cc:313: >> > ASSERT_TRUE(response.find("Response!") > >> > != std::string::npos); >> > On 2014/03/27 20:59:46, Ryan Sleevi wrote: >> > > ASSERT_NE(std::string::npos, response.find("Response!")) ? >> > >> > Done. >> > > Ping? >> > > Sorry for forgetting Jesse's request. > This api is necessary as Jesse already mentioned to send back via http > protocol > 1) /etc/LICENSE.html.gz to show license terms applied in the device which > is > about 120Kbytes > 2) logging messages which could be more than 1Mbytes > > 1) is inevitable because of open license policy. > 2) is not enabled for production, but still useful to debug the device > with eng > build. > > https://codereview.chromium.org/212683006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I think that would work, yes; it shouldn't be about sending the raw headers as much as sending a constructed header followed by pieces of a data stream. What do you envision that API looking like? On 2014/04/02 15:08:37, Ryan Sleevi wrote: > Thank you for explaining, byungchul. > > I agree that this API probably meets those goals, but my feeling is that it > does so fairly uncleanly - exposing the 'raw' Send method is somewhat of an > abstraction cop-out. > > I will take another look at this, but it seems your requirements could also > be met if you were allowed to send incremental data, is that correct? > > That is, you have no need for manually manipulating or constructing the > response or headers, you just wish to be able to stream data? > > Have I missed something in your requirements? > On Apr 2, 2014 7:58 AM, <mailto:byungchul@chromium.org> wrote: > > > On 2014/04/02 00:57:49, gunsch wrote: > > > >> On 2014/03/27 23:41:16, gunsch wrote: > >> > > >> > > > > https://codereview.chromium.org/212683006/diff/40001/net/ > > server/http_server_unittest.cc > > > >> > File net/server/http_server_unittest.cc (right): > >> > > >> > > >> > > > > https://codereview.chromium.org/212683006/diff/40001/net/ > > server/http_server_unittest.cc#newcode137 > > > >> > net/server/http_server_unittest.cc:137: base::Closure > >> run_loop_quit_func_; > >> > On 2014/03/27 20:59:46, Ryan Sleevi wrote: > >> > > You shouldn't need this separate RunLoop helper. You should be able > >> to use > >> > > TestCompletionCallback w/ WaitForResult() to handle just this. > >> > > >> > Done, thanks for the tip. That's a lot simpler. > >> > > >> > > >> > > > > https://codereview.chromium.org/212683006/diff/80001/net/ > > server/http_server_unittest.cc > > > >> > File net/server/http_server_unittest.cc (right): > >> > > >> > > >> > > > > https://codereview.chromium.org/212683006/diff/80001/net/ > > server/http_server_unittest.cc#newcode123 > > > >> > net/server/http_server_unittest.cc:123: void Read(const base::Closure& > >> > quit_loop) { > >> > On 2014/03/27 20:59:46, Ryan Sleevi wrote: > >> > > This overloads with the Read method on 92. Rename this something like > >> > > ReadInternal? > >> > > >> > Done. > >> > > >> > > >> > > > > https://codereview.chromium.org/212683006/diff/80001/net/ > > server/http_server_unittest.cc#newcode191 > > > >> > net/server/http_server_unittest.cc:191: HttpServerRequestInfo > >> request(int > >> > request_index) { > >> > On 2014/03/27 20:59:46, Ryan Sleevi wrote: > >> > > naming: GetRequest() > >> > > defensive: Should you DCHECK_LE(request_index, requests_.size()) > >> > > type safety: size_t request_index, not int. > >> > > >> > Done. > >> > > >> > > >> > > > > https://codereview.chromium.org/212683006/diff/80001/net/ > > server/http_server_unittest.cc#newcode195 > > > >> > net/server/http_server_unittest.cc:195: int connection_id(int > >> request_index) > >> > > { > > > >> > On 2014/03/27 20:59:46, Ryan Sleevi wrote: > >> > > naming: GetConnectionId() > >> > > defensive: Should you DCHECK_LE(request_index, requests_.size()) > >> > > type safety: size_t request_index, not int. > >> > > >> > Done. > >> > > >> > > >> > > > > https://codereview.chromium.org/212683006/diff/80001/net/ > > server/http_server_unittest.cc#newcode313 > > > >> > net/server/http_server_unittest.cc:313: > >> > > ASSERT_TRUE(response.find("Response!") > > > >> > != std::string::npos); > >> > On 2014/03/27 20:59:46, Ryan Sleevi wrote: > >> > > ASSERT_NE(std::string::npos, response.find("Response!")) ? > >> > > >> > Done. > >> > > > > Ping? > >> > > > > Sorry for forgetting Jesse's request. > > This api is necessary as Jesse already mentioned to send back via http > > protocol > > 1) /etc/LICENSE.html.gz to show license terms applied in the device which > > is > > about 120Kbytes > > 2) logging messages which could be more than 1Mbytes > > > > 1) is inevitable because of open license policy. > > 2) is not enabled for production, but still useful to debug the device > > with eng > > build. > > > > https://codereview.chromium.org/212683006/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Sorry for the delays, I was out sick. On the upside, having thought about it more, I think we can proceed with this current path. A more flexible design would be in allowing the (creator) of the HttpServerResponseInfo to supply some sort of Streaming response object to stream the body. However, that's an added layer of complexity we need not introduce right now. I think I'm comfortable with the current approach of "raw" sending, even though it's less than ideal. https://codereview.chromium.org/212683006/diff/100001/net/server/http_server_... File net/server/http_server_unittest.cc (right): https://codereview.chromium.org/212683006/diff/100001/net/server/http_server_... net/server/http_server_unittest.cc:97: std::string io_buffer_contents(read_buffer_->data()); FLAKINESS BUG: You don't check that bytes_received > 0 here
On 2014/04/07 17:07:24, Ryan Sleevi wrote: > Sorry for the delays, I was out sick. > > On the upside, having thought about it more, I think we can proceed with this > current path. A more flexible design would be in allowing the (creator) of the > HttpServerResponseInfo to supply some sort of Streaming response object to > stream the body. However, that's an added layer of complexity we need not > introduce right now. I think I'm comfortable with the current approach of "raw" > sending, even though it's less than ideal. > > https://codereview.chromium.org/212683006/diff/100001/net/server/http_server_... > File net/server/http_server_unittest.cc (right): > > https://codereview.chromium.org/212683006/diff/100001/net/server/http_server_... > net/server/http_server_unittest.cc:97: std::string > io_buffer_contents(read_buffer_->data()); > FLAKINESS BUG: You don't check that bytes_received > 0 here Sounds good, thanks for taking a look at this again.
https://codereview.chromium.org/212683006/diff/120001/net/server/http_server_... File net/server/http_server_unittest.cc (right): https://codereview.chromium.org/212683006/diff/120001/net/server/http_server_... net/server/http_server_unittest.cc:97: EXPECT_GT(bytes_received, 0); This won't do what you want. When this condition fails, the test will fail, yes, but it will continue executing. When it gets to line 99, it's going to request a substr(0, -some-negative-number), which won't do what you want. Handle this here (eg: if (bytes_received < 0) return std::string()), it seems, and EXPECT_()/ASSERT_() in the calling function? https://codereview.chromium.org/212683006/diff/120001/net/server/http_server_... net/server/http_server_unittest.cc:184: EXPECT_LE(request_index, requests_.size()); This doesn't really do what you want, for similar reasons as above. When you hit line 185, you're going to crash the test if request_index > requests_.size() Better to handle it here, or move this check into the calling function? (Note: Don't replace these with DCHECK() - it would have the same effect for test flakiness, as it would crash the test) Normally you'd use ASSERT_LE(), but that requires a void return, and will only terminate the test if it's in the test body (otherwise it would just "return") see https://code.google.com/p/googletest/wiki/AdvancedGuide#Propagating_Fatal_Fai... to understand the patterns https://codereview.chromium.org/212683006/diff/120001/net/server/http_server_... net/server/http_server_unittest.cc:189: EXPECT_LE(request_index, requests_.size()); Ditto; this doesn't do what you expect.
https://codereview.chromium.org/212683006/diff/120001/net/server/http_server_... File net/server/http_server_unittest.cc (right): https://codereview.chromium.org/212683006/diff/120001/net/server/http_server_... net/server/http_server_unittest.cc:97: EXPECT_GT(bytes_received, 0); On 2014/04/10 20:58:36, Ryan Sleevi wrote: > This won't do what you want. > > When this condition fails, the test will fail, yes, but it will continue > executing. When it gets to line 99, it's going to request a substr(0, > -some-negative-number), which won't do what you want. > > Handle this here (eg: if (bytes_received < 0) return std::string()), it seems, > and EXPECT_()/ASSERT_() in the calling function? Interesting, guess I was still expecting the Java/JUnit style of "drop an assert anywhere and the test exits if it fails." Updated. https://codereview.chromium.org/212683006/diff/120001/net/server/http_server_... net/server/http_server_unittest.cc:184: EXPECT_LE(request_index, requests_.size()); On 2014/04/10 20:58:36, Ryan Sleevi wrote: > This doesn't really do what you want, for similar reasons as above. When you hit > line 185, you're going to crash the test if request_index > requests_.size() > > Better to handle it here, or move this check into the calling function? > > (Note: Don't replace these with DCHECK() - it would have the same effect for > test flakiness, as it would crash the test) > > Normally you'd use ASSERT_LE(), but that requires a void return, and will only > terminate the test if it's in the test body (otherwise it would just "return") > > see > https://code.google.com/p/googletest/wiki/AdvancedGuide#Propagating_Fatal_Fai... > to understand the patterns In that case, I'd rather remove these checks----it's a simple array index, and the calling tests are already asserting that N requests were successfully made (ASSERT_TRUE(RunUntilRequestsReceived)). I don't think propagating a success boolean adds anything to the test for the extra complexity. Thanks for the link though, that helps with the test paradigms generally. https://codereview.chromium.org/212683006/diff/120001/net/server/http_server_... net/server/http_server_unittest.cc:189: EXPECT_LE(request_index, requests_.size()); On 2014/04/10 20:58:36, Ryan Sleevi wrote: > Ditto; this doesn't do what you expect. Done.
lgtm
The CQ bit was checked by gunsch@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gunsch@chromium.org/212683006/140001
The CQ bit was unchecked by commit-bot@chromium.org
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
The CQ bit was checked by gunsch@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gunsch@chromium.org/212683006/140001
Message was sent while issue was closed.
Change committed as 263948
Message was sent while issue was closed.
Looks like this is making the ASAN bots sad. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
