Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
249031:
Refactor HttpNetworkTransaction: Parse stream in HttpStream (Closed)
|
Created:
11 years, 2 months ago by vandebo (ex-Chrome) Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com, darin (slow to review), brettw, willchan no longer on Chromium, Paweł Hajdan Jr. Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionRefactor HttpNetworkTransaction so that HttpStream is responsible for parsing the Http traffic. HttpBasicStream delegates parsing to HttpStreamParser in preparation for HttpPipelinedStream.
BUG=13289
TEST=unittests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=29316
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 63
Patch Set 6 : '' #Patch Set 7 : '' #Patch Set 8 : '' #
Total comments: 40
Patch Set 9 : '' #Patch Set 10 : '' #
Total comments: 54
Patch Set 11 : '' #
Total comments: 4
Patch Set 12 : '' #
Messages
Total messages: 15 (0 generated)
This change deserves more testing (testing suggestions welcome), but it passes unit tests and I'm ready for review feedback. Feel free to suggest alternate reviewers, you two are the biggest contributors to HttpNetworkTransaction.
> I know this is a large review, but I'm wondering how it's coming along. I am doing the review now too (sorry was swamped last week).
a couple initial nits http://codereview.chromium.org/249031/diff/8001/8018 File net/base/io_buffer.h (right): http://codereview.chromium.org/249031/diff/8001/8018#newcode85 Line 85: explicit StringIOBuffer(std::string s) const std::string& http://codereview.chromium.org/249031/diff/8001/8018#newcode86 Line 86: : IOBuffer(static_cast<char*>(NULL)), why the static-cast? http://codereview.chromium.org/249031/diff/8001/8018#newcode88 Line 88: data_ = const_cast<char*>(string_data_.data()); why the const-cast ? http://codereview.chromium.org/249031/diff/8001/8006 File net/http/http_basic_stream.cc (right): http://codereview.chromium.org/249031/diff/8001/8006#newcode16 Line 16: read_buf_ = new GrowableIOBuffer(); can you just move these initializations into the initializer list? http://codereview.chromium.org/249031/diff/8001/8008 File net/http/http_basic_stream.h (right): http://codereview.chromium.org/249031/diff/8001/8008#newcode33 Line 33: std::string headers, const std::string& http://codereview.chromium.org/249031/diff/8001/8010 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/249031/diff/8001/8010#newcode1349 Line 1349: // BuildRequestHeaders to be called, which rewinds request_body_stream_ to request_body_stream_ no longer exists. http://codereview.chromium.org/249031/diff/8001/8007 File net/http/http_network_transaction_unittest.cc (right): http://codereview.chromium.org/249031/diff/8001/8007#newcode3605 Line 3605: // Make sure we can handle an error when writing the request nit: end with period. http://codereview.chromium.org/249031/diff/8001/8007#newcode3633 Line 3633: // Check that a connection closed after the start of the headers finishes ok nit: end with period. http://codereview.chromium.org/249031/diff/8001/8002 File net/http/http_stream.h (right): http://codereview.chromium.org/249031/diff/8001/8002#newcode63 Line 63: virtual int64 BytesRemainingInBody() const = 0; This interface is confusing to me; I think it would be better if the return value isn't mixed with the error conditions... How about: int GetBytesRemainingInBody(int64* remaining); (the return value can describe success/fail, and the bytes are set in |*remaining|. http://codereview.chromium.org/249031/diff/8001/8012 File net/http/http_stream_parser.cc (right): http://codereview.chromium.org/249031/diff/8001/8012#newcode1 Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved. Please use "svn cp" to preserve the version history. Also it will make the diffs easier to read, since this won't show up as all new code... http://codereview.chromium.org/249031/diff/8001/8012#newcode41 Line 41: DCHECK(io_state_ == NONE); DCHECK_EQ http://codereview.chromium.org/249031/diff/8001/8012#newcode60 Line 60: DCHECK(io_state_ == REQUEST_SENT); DCHECK_EQ http://codereview.chromium.org/249031/diff/8001/8013 File net/http/http_stream_parser.h (right): http://codereview.chromium.org/249031/diff/8001/8013#newcode30 Line 30: explicit HttpStreamParser(ClientSocketHandle* handle, no need for explicit if there is more than 1 argument. http://codereview.chromium.org/249031/diff/8001/8013#newcode36 Line 36: int SendRequest(const HttpRequestInfo* request, std::string headers, const std::string& headers
Some general comments first. Nit: When documenting member functions in a header, be consistent in using either a tbird person verb (Gets, Returns, etc.) or a command voice (Get, Return, etc.). We usually use a third person verb in that context, but please follow the prevalent style in the file you're modifying. Nit: Please add a period at the end of a comment, especially if it's a sentence. (We also do that when it's a sentence fragment.) General comment: You changed how the state machine is implemented, but the change seems to be superficial (renaming the functions and data members, etc.), and it's not clear if it's better than the original code. Unless there is a strong reason to change, I'd prefer that you continue to use the current way of implementing the state machine. http://codereview.chromium.org/249031/diff/8001/8017 File net/base/io_buffer.cc (right): http://codereview.chromium.org/249031/diff/8001/8017#newcode20 Line 20: void DrainableIOBuffer::DidConsume(size_t bytes) { Here you can see the similarity between ReusedIOBuffer::SetOffset and DrainableIOBuffer::DidConsume. I hope we can combine the two classes. http://codereview.chromium.org/249031/diff/8001/8018 File net/base/io_buffer.h (right): http://codereview.chromium.org/249031/diff/8001/8018#newcode83 Line 83: class StringIOBuffer : public IOBuffer { Please document the three new IOBuffer classes. http://codereview.chromium.org/249031/diff/8001/8018#newcode101 Line 101: class DrainableIOBuffer : public IOBuffer { Are you sure you can't use ReusedIOBuffer? This seems similar. http://codereview.chromium.org/249031/diff/8001/8018#newcode103 Line 103: DrainableIOBuffer(IOBuffer* base, size_t size) Use 'int' instead of 'size_t' to be consistent with existing IOBuffer classes in this file. http://codereview.chromium.org/249031/diff/8001/8008 File net/http/http_basic_stream.h (left): http://codereview.chromium.org/249031/diff/8001/8008#oldcode23 Line 23: // HttpStream methods: Please resurrect this comment. In the Chromium source tree we use such comments to mark the methods that come from a base class. http://codereview.chromium.org/249031/diff/8001/8008 File net/http/http_basic_stream.h (right): http://codereview.chromium.org/249031/diff/8001/8008#newcode24 Line 24: class HttpStreamParser; Since you include "net/http/http_stream_parser.h", you should not need the forward declaration of HttpStreamParser. http://codereview.chromium.org/249031/diff/8001/8011 File net/http/http_chunked_decoder.cc (right): http://codereview.chromium.org/249031/diff/8001/8011#newcode76 Line 76: bytes_after_eof_ = buf_len; Should this be bytes_after_eof_ += buf_len; (note the +=) instead? Isn't it a programming error to call FilterBuf after the decoder has reached EOF? http://codereview.chromium.org/249031/diff/8001/8004 File net/http/http_chunked_decoder.h (right): http://codereview.chromium.org/249031/diff/8001/8004#newcode87 Line 87: int bytes_after_eof() const { return reached_eof_ ? bytes_after_eof_ : 0; } Nit: list this method below the reached_eof() method because they're both all lowercase. http://codereview.chromium.org/249031/diff/8001/8009 File net/http/http_chunked_decoder_unittest.cc (right): http://codereview.chromium.org/249031/diff/8001/8009#newcode17 Line 17: int bytes_after) { Nit: bytes_after => bytes_after_eof http://codereview.chromium.org/249031/diff/8001/8003 File net/http/http_network_transaction.h (right): http://codereview.chromium.org/249031/diff/8001/8003#newcode306 Line 306: // Track the validness of the response headers Nit: validness => validity http://codereview.chromium.org/249031/diff/8001/8002 File net/http/http_stream.h (right): http://codereview.chromium.org/249031/diff/8001/8002#newcode16 Line 16: #include "net/socket/client_socket_handle.h" Do we really need to include client_socket_handle.h? http://codereview.chromium.org/249031/diff/8001/8002#newcode31 Line 31: // Writes the headers and uploads body data to the socket. ERR_IO_PENDING The comments in this file often mention "the socket". Do you really mean "the HTTP stream"? http://codereview.chromium.org/249031/diff/8001/8002#newcode34 Line 34: // caller takes ownership of the request_body. Did you mean "the caller" or "the function"? http://codereview.chromium.org/249031/diff/8001/8002#newcode36 Line 36: std::string headers, This parameter should be a const reference. http://codereview.chromium.org/249031/diff/8001/8002#newcode47 Line 47: // ResponseInfo provides access to the response headers, etc. ResponseInfo => HttpResponseInfo. Please document the ownership of the returned HttpResponseInfo. http://codereview.chromium.org/249031/diff/8001/8002#newcode50 Line 50: // Reads body data, up to buf_len bytes. The number of bytes read is "body data" => "response body data". http://codereview.chromium.org/249031/diff/8001/8002#newcode61 Line 61: // IO_PENDING indicates the size is to be determined (chunked encoding) IO_PENDING => ERR_IO_PENDING. It is strange to overload ERR_IO_PENDING for this purpose. Feel free to add new error codes or enum constants for this function. http://codereview.chromium.org/249031/diff/8001/8002#newcode62 Line 62: // FAILED indicates that we can not determine the size FAILED => ERR_FAILED Please add a new error code or enum constant for this condition. Note: I studied how BytesRemainingInBody() is used. The actual byte count is not important. Perhaps you can invent an enum type for the return value. Nit: Please add periods (.) after the two sentences in this comment block. http://codereview.chromium.org/249031/diff/8001/8002#newcode65 Line 65: virtual bool IsMoreDataBuffered() const = 0; Please document this method. http://codereview.chromium.org/249031/diff/8001/8012 File net/http/http_stream_parser.cc (right): http://codereview.chromium.org/249031/diff/8001/8012#newcode95 Line 95: if (scheduled_callback_) { We don't need a separate scheduled_callback_ member. In the original code, we call the client callback if DoLoop() returns a result that's not ERR_IO_PENDING. result != ERR_IO_PENDING is how we know we're done. See HttpNetworkTransaction::DoLoop. Why don't you do that here? http://codereview.chromium.org/249031/diff/8001/8012#newcode104 Line 104: bool can_do_more = true; can_do_more == false is equivalent to next_state_ == STATE_NONE elsewhere in the net module. Would be nice to be consistent with the existing code. http://codereview.chromium.org/249031/diff/8001/8012#newcode343 Line 343: status = chunked_decoder_->FilterBuf(body_buf_->data(), status); Rather than adding the bytes_after_eof() method, you may be able to skip calling FilterBuf() if reached_eof() already returns true. You can save that fact in a bool member variable. http://codereview.chromium.org/249031/diff/8001/8012#newcode432 Line 432: void HttpStreamParser::CalculateBodySize() { Nit: rename this method CalculateResponseBodyLength to be consistent with the response_body_length_ member. http://codereview.chromium.org/249031/diff/8001/8012#newcode498 Line 498: else if (response_body_length_ == -1) Nit: In the net module, we don't use "else" after a return statement. http://codereview.chromium.org/249031/diff/8001/8013 File net/http/http_stream_parser.h (right): http://codereview.chromium.org/249031/diff/8001/8013#newcode22 Line 22: class HttpStreamParser { General comment: since the request may have a body (the upload/POST data), where there isn't enough context to tell whether "body" refers to the request or response body, you should use ResponseBody or response_body instead of just Body or body. http://codereview.chromium.org/249031/diff/8001/8013#newcode36 Line 36: int SendRequest(const HttpRequestInfo* request, std::string headers, The headers paramater should be a const reference. Nit: we're supposed to format function declarations and definitions in one of the forms recommended by the style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla... Note that the style guide recommends slightly different formatting for function calls: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls http://codereview.chromium.org/249031/diff/8001/8013#newcode52 Line 52: NONE, Please add STATE_ or IO_STATE_ prefix to these enumeration constants, so they can be easily recognized as the name of a state. http://codereview.chromium.org/249031/diff/8001/8013#newcode75 Line 75: void OnIOReady(int status); This function is called OnIOComplete in some other files in the net module. It'd be nice to use the same naming convention unless there is a strong reason to deviate. Similarly, the int parameter is named 'result' elsewhere in the net module. http://codereview.chromium.org/249031/diff/8001/8013#newcode78 Line 78: int DoPendingIO(int status); Same comment: this function is named DoLoop elsewhere in the net module. http://codereview.chromium.org/249031/diff/8001/8013#newcode95 Line 95: // Parse the headers into response_info_ Typo: response_info_ => response_. http://codereview.chromium.org/249031/diff/8001/8013#newcode141 Line 141: CompletionCallback* client_callback_; Nit: elsewhere in the net module, we call this the "user" callback. http://codereview.chromium.org/249031/diff/8001/8013#newcode150 Line 150: ClientSocketHandle* const handle_; Let's name this member socket_ or socket_handle_, or even connection_, which is the name of the member in the original code. http://codereview.chromium.org/249031/diff/8001/8016 File net/socket/client_socket_handle.h (right): http://codereview.chromium.org/249031/diff/8001/8016#newcode107 Line 107: bool should_resend_failed_request(int error) const { It's good to create a method for this code, but this method does not belong in this class. It should be in an HTTP-related class.
All feedback integrated or responded to. wtc: wrt your general comment. I changed the superficial renamings back, for the sake of consistency, though some of those names are mildly misleading. But the state machine does function a bit differently. Part of that is to provide better safety because there is a different interface to the state machine (more entry points) and part of it is to be more explicit about what is happening in order to make it easier to understand. http://codereview.chromium.org/249031/diff/8001/8017 File net/base/io_buffer.cc (right): http://codereview.chromium.org/249031/diff/8001/8017#newcode20 Line 20: void DrainableIOBuffer::DidConsume(size_t bytes) { On 2009/10/05 22:48:27, wtc wrote: > Here you can see the similarity between ReusedIOBuffer::SetOffset > and DrainableIOBuffer::DidConsume. I hope we can > combine the two classes. A quick search show three used of ReusedIOBuffer, two of which have additional state, which can be trivially eliminated by using DrainableIOBuffer instead. How about I combine them by converting all of them to the Drainable style. (FYI I don't think either name is great.) http://codereview.chromium.org/249031/diff/8001/8018 File net/base/io_buffer.h (right): http://codereview.chromium.org/249031/diff/8001/8018#newcode86 Line 86: : IOBuffer(static_cast<char*>(NULL)), On 2009/10/05 21:08:43, eroman wrote: > why the static-cast? It was ambiguous at some point, seems to be fine without the cast now. Done. http://codereview.chromium.org/249031/diff/8001/8018#newcode88 Line 88: data_ = const_cast<char*>(string_data_.data()); On 2009/10/05 21:08:43, eroman wrote: > why the const-cast ? The base class defines data_ as char*. I added a comment to make it clear how this subclass can be used. http://codereview.chromium.org/249031/diff/8001/8018#newcode101 Line 101: class DrainableIOBuffer : public IOBuffer { On 2009/10/05 22:48:27, wtc wrote: > Are you sure you can't use ReusedIOBuffer? This seems similar. In principle, I could use ReusedIOBuffer, but it would require keeping additional state. By encapsulating that state in DrainableIOBuffer, a more readable interface is possible. if (buffer_.size() - buffer_bytes_used) becomes if (buffer_.BytesRemaining()) and buffer_bytes_used_ += status; buffer_.SetOffset(buffer_bytes_used_); becomes buffer_.DidConsume(status) http://codereview.chromium.org/249031/diff/8001/8018#newcode103 Line 103: DrainableIOBuffer(IOBuffer* base, size_t size) On 2009/10/05 22:48:27, wtc wrote: > Use 'int' instead of 'size_t' to be consistent with existing > IOBuffer classes in this file. But size_t is the right type to use here. It can't be negative and is expressly meant to represent the number of bytes in things, so should be larger than an int before objects commonly exceed 2GB in size. Growable and Drainable are abstractions of one-off's that were in HttpNetworkTransaction.h (which seemed like nice general purpose interfaces), so I would prefer to get the interface right if possible. But I will change these to ints if you feel strongly that consistency should prevail. http://codereview.chromium.org/249031/diff/8001/8011 File net/http/http_chunked_decoder.cc (right): http://codereview.chromium.org/249031/diff/8001/8011#newcode76 Line 76: bytes_after_eof_ = buf_len; On 2009/10/05 22:48:27, wtc wrote: > Should this be > bytes_after_eof_ += buf_len; > (note the +=) instead? > > Isn't it a programming error to call FilterBuf after the > decoder has reached EOF? The header isn't explicit on the matter. It implies that it is permissible to call FilterBuf() multiple times before checking reached_eof(). In which case, my addition needs to be a +=. Good catch! Added a unit test to catch this. http://codereview.chromium.org/249031/diff/8001/8002 File net/http/http_stream.h (right): http://codereview.chromium.org/249031/diff/8001/8002#newcode16 Line 16: #include "net/socket/client_socket_handle.h" On 2009/10/05 22:48:27, wtc wrote: > Do we really need to include client_socket_handle.h? All classes that use HttpStream will need to include it, and include order becomes important if it's not here. http://codereview.chromium.org/249031/diff/8001/8002#newcode31 Line 31: // Writes the headers and uploads body data to the socket. ERR_IO_PENDING On 2009/10/05 22:48:27, wtc wrote: > The comments in this file often mention "the socket". Do > you really mean "the HTTP stream"? It depends on how you look at it. This interface adapts a socket (with its read/write interface) to a stream of HTTP requests. The latter to references definitely refer to a socket. http://codereview.chromium.org/249031/diff/8001/8002#newcode34 Line 34: // caller takes ownership of the request_body. On 2009/10/05 22:48:27, wtc wrote: > Did you mean "the caller" or "the function"? Fixed. http://codereview.chromium.org/249031/diff/8001/8002#newcode63 Line 63: virtual int64 BytesRemainingInBody() const = 0; On 2009/10/05 21:08:43, eroman wrote: > This interface is confusing to me; I think it would be better if the return > value isn't mixed with the error conditions... How about: > > int GetBytesRemainingInBody(int64* remaining); > > (the return value can describe success/fail, and the bytes are set in > |*remaining|. As wtc points out, the amount of data remaining isn't actually important. I've replaced this function with too predicates: bool IsResponseBodyComplete(); bool CanFindEndOfResponse(); http://codereview.chromium.org/249031/diff/8001/8012 File net/http/http_stream_parser.cc (right): http://codereview.chromium.org/249031/diff/8001/8012#newcode95 Line 95: if (scheduled_callback_) { On 2009/10/05 22:48:27, wtc wrote: > We don't need a separate scheduled_callback_ member. > > In the original code, we call the client callback if > DoLoop() returns a result that's not ERR_IO_PENDING. > result != ERR_IO_PENDING is how we know we're done. > See HttpNetworkTransaction::DoLoop. > > Why don't you do that here? I tried to minimize the number of behavioral changes, so the state machine doesn't allow it. For instance, DoProessBody needs to handle some error conditions. http://codereview.chromium.org/249031/diff/8001/8012#newcode104 Line 104: bool can_do_more = true; On 2009/10/05 22:48:27, wtc wrote: > can_do_more == false is equivalent to next_state_ == STATE_NONE > elsewhere in the net module. Would be nice to be consistent > with the existing code. With STATE_NONE, you actually lose the state and depend on the caller to callback into the state machine correctly. In HttpNetworkTransaction, this only happens once - transitioning from ReadHeadersComplete to ReadBody, so it is less of an issue. But that transition is confusing because it is unlike all the others. In HttpStream, there are three points where the caller gets control back and could call into HttpStream at the wrong place. Furthermore, can_do_more and schedule_callback are more explicit and make the state machine easier to follow. http://codereview.chromium.org/249031/diff/8001/8012#newcode343 Line 343: status = chunked_decoder_->FilterBuf(body_buf_->data(), status); On 2009/10/05 22:48:27, wtc wrote: > Rather than adding the bytes_after_eof() method, you may be > able to skip calling FilterBuf() if reached_eof() already > returns true. You can save that fact in a bool member > variable. The data after EOF may have come in the same read() as the EOF indicator, so I would still need to know if there was any extra data. http://codereview.chromium.org/249031/diff/8001/8013 File net/http/http_stream_parser.h (right): http://codereview.chromium.org/249031/diff/8001/8013#newcode75 Line 75: void OnIOReady(int status); On 2009/10/05 22:48:27, wtc wrote: > This function is called OnIOComplete in some other files in > the net module. It'd be nice to use the same naming > convention unless there is a strong reason to deviate. > > Similarly, the int parameter is named 'result' elsewhere in > the net module. Part of the renaming was me understanding the state machine in HttpNetworkTransaction. OnIOComplete was unintuitive because it implies that IO is done and over with. Changed to OnIOComplete because consistency wins here. http://codereview.chromium.org/249031/diff/8001/8016 File net/socket/client_socket_handle.h (right): http://codereview.chromium.org/249031/diff/8001/8016#newcode107 Line 107: bool should_resend_failed_request(int error) const { On 2009/10/05 22:48:27, wtc wrote: > It's good to create a method for this code, but this method > does not belong in this class. It should be in an HTTP-related > class. Semantically, it may be http related, but code-wise it fits well here: most of the predicates are on connection state. It was in HttpNetworkTransaction, but I needed it from HttpStreamParser as well. ClientSocketHandle seemed like the logical place to move it to. Do you have an alternate suggestion?
http://codereview.chromium.org/249031/diff/8001/8018 File net/base/io_buffer.h (right): http://codereview.chromium.org/249031/diff/8001/8018#newcode86 Line 86: : IOBuffer(static_cast<char*>(NULL)), On 2009/10/05 21:08:43, eroman wrote: > why the static-cast? Oops, this is actually needed to get the right constructor - compiler confuses NULL for an int.
Steve, Sorry, I tried to review as much as I could, but this is a big CL. Here is what I reviewed today. I also found that you didn't make some of the changes I suggested last time. I didn't read your reply to my review comments, so perhaps you dropped those changes after considering them. I just wanted to make sure you didn't miss those changes. http://codereview.chromium.org/249031/diff/12001/5015 File net/http/http_basic_stream.h (left): http://codereview.chromium.org/249031/diff/12001/5015#oldcode23 Line 23: // HttpStream methods: Please resurrect this comment. This comment is a convention in the Chromium source tree. http://codereview.chromium.org/249031/diff/12001/5011 File net/http/http_chunked_decoder.h (right): http://codereview.chromium.org/249031/diff/12001/5011#newcode115 Line 115: // The number of unfiltered bytes after the final CRLF. Please add a comment to explain that these bytes are part of the next response. It took me a while to figure this out. http://codereview.chromium.org/249031/diff/12001/5017 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/249031/diff/12001/5017#newcode797 Line 797: // TODO(wtc): Traditionally this code has returned 0 when reading a closed We should continue to report EOF with a 0 return value at the external interface. It's OK to use an error code to indicate EOF internally. Rather than recycling ERR_CONNECTION_CLOSED (which was added in error actually), we probably should add a new internal error code ERR_EOF. http://codereview.chromium.org/249031/diff/12001/5009 File net/http/http_stream.h (right): http://codereview.chromium.org/249031/diff/12001/5009#newcode21 Line 21: class HttpRequestInfo; Delete the duplicate forward declaration for class HttpRequestInfo. http://codereview.chromium.org/249031/diff/12001/5009#newcode31 Line 31: // Writes the headers and uploads body data to the underlying socket. Define what the return value is. I guess it's the number of bytes sent, or a network error code on failure. http://codereview.chromium.org/249031/diff/12001/5009#newcode45 Line 45: virtual int ReadResponseHeaders(CompletionCallback* callback) = 0; Define the return value. Does it return the byte count or OK on success? http://codereview.chromium.org/249031/diff/12001/5009#newcode51 Line 51: // is returned, or an error is returned upon failure. ERR_CONNECTION_CLOSED Can we avoid returning ERR_CONNECTION_CLOSED for EOF here? This is a public method, In external interface of a class, we should try to report EOF using the conventional 0 return value. http://codereview.chromium.org/249031/diff/12001/5009#newcode65 Line 65: // length. If neither is sent, the connection must close in order to Nit: change the connection must close ... to the server must close the connection for us to find the end of the response. http://codereview.chromium.org/249031/diff/12001/5009#newcode70 Line 70: // data has been read from the socket. Add "(part of the next response)" after "data". http://codereview.chromium.org/249031/diff/12001/5019 File net/http/http_stream_parser.cc (right): http://codereview.chromium.org/249031/diff/12001/5019#newcode78 Line 78: if (io_state_ == STATE_DONE) Why don't you use the solution in HttpNetworkTransaction::Read() and HttpNetworkTransaction::DoReadBody()? if (!connection_.is_initialized()) return 0; // connection_ has been reset. Treat like EOF. and // We may have already consumed the indicated content length. if (response_body_length_ != -1 && response_body_read_ >= response_body_length_) return 0; Then we can get rid of the STATE_BODY_PENDING and STATE_DONE states. http://codereview.chromium.org/249031/diff/12001/5019#newcode127 Line 127: case STATE_REQUEST_SENT: In the original http_network_transaction.cc code, we just set next_state_ to STATE_NONE, which will cause us to break out of the do-while loop. Are you sure you don't want to do that? http://codereview.chromium.org/249031/diff/12001/5019#newcode157 Line 157: NOTREACHED(); We need to set can_do_more to false, otherwise we will be stuck in this loop. http://codereview.chromium.org/249031/diff/12001/5019#newcode161 Line 161: You always call ScheduleCallback() before you set can_do_more to false. So, you can just call ScheduleCallback() here, as follows: if (result != ERR_IO_PENDING) ScheduleCallback(); This will require you to set result to OK in STATE_REQUEST_SENT, STATE_BODY_PENDING, and STATE_DONE. http://codereview.chromium.org/249031/diff/12001/5019#newcode174 Line 174: result = connection_->socket()->Write(request_headers_, If the Write() call returns a real error (not ERR_IO_PENDING), you will go back to DoSendHeaders with a negative 'result'. Then things will go wrong because you aren't checking if result < 0. This comment also applies to DoSendBody. http://codereview.chromium.org/249031/diff/12001/5020 File net/http/http_stream_parser.h (right): http://codereview.chromium.org/249031/diff/12001/5020#newcode34 Line 34: // These functions implement the interface described in HttpStream with Just wondering: why doesn't this class derive from HttpStream then? http://codereview.chromium.org/249031/diff/12001/5020#newcode61 Line 61: STATE_PROCESSING_HEADERS, The renaming of the STATE_READ_HEADERS_COMPLETE to STATE_PROCESSING_HEADERS and STATE_READ_BODY_COMPLETE to STATE_PROCESSING_BODY seems gratuitous. It will confuse people like me who are familiar with the original code, with little gain. http://codereview.chromium.org/249031/diff/12001/5020#newcode137 Line 137: scoped_refptr<IOBuffer> body_buf_; Nit: please rename these members response_body_buf_ and response_body_buf_len_. http://codereview.chromium.org/249031/diff/12001/5020#newcode148 Line 148: CompletionCallback* scheduled_callback_; I still don't understand why you need scheduled_callback_ here, when we can get by without it elsewhere.
Response to your additional comments below. Any changes from the first round that I didn't take, I should have responded to. I know this is a large patch that is difficult to follow from old code to new code. I appreciate all the feedback. I hope you don't find my pushback too argumentative, I made some changes to try to improve the readability of the code. http://codereview.chromium.org/249031/diff/12001/5015 File net/http/http_basic_stream.h (left): http://codereview.chromium.org/249031/diff/12001/5015#oldcode23 Line 23: // HttpStream methods: On 2009/10/09 02:22:39, wtc wrote: > Please resurrect this comment. This comment is a convention > in the Chromium source tree. Done. http://codereview.chromium.org/249031/diff/12001/5011 File net/http/http_chunked_decoder.h (right): http://codereview.chromium.org/249031/diff/12001/5011#newcode115 Line 115: // The number of unfiltered bytes after the final CRLF. On 2009/10/09 02:22:39, wtc wrote: > Please add a comment to explain that these bytes are part of > the next response. It took me a while to figure this out. Done. http://codereview.chromium.org/249031/diff/12001/5017 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/249031/diff/12001/5017#newcode797 Line 797: // TODO(wtc): Traditionally this code has returned 0 when reading a closed On 2009/10/09 02:22:39, wtc wrote: > We should continue to report EOF with a 0 return value > at the external interface. > > It's OK to use an error code to indicate EOF internally. > Rather than recycling ERR_CONNECTION_CLOSED (which was > added in error actually), we probably should add a new > internal error code ERR_EOF. In this context, getting an EOF means that the connection was closed. Do the existing uses of ERR_CONNECTION_CLOSED mean something else? If so, maybe that use should get renamed. ERR_EOF is less descriptive and could be interpreted ambiguously, since we send files over HTTP. http://codereview.chromium.org/249031/diff/12001/5009 File net/http/http_stream.h (right): http://codereview.chromium.org/249031/diff/12001/5009#newcode21 Line 21: class HttpRequestInfo; On 2009/10/09 02:22:39, wtc wrote: > Delete the duplicate forward declaration for class HttpRequestInfo. Done. http://codereview.chromium.org/249031/diff/12001/5009#newcode31 Line 31: // Writes the headers and uploads body data to the underlying socket. On 2009/10/09 02:22:39, wtc wrote: > Define what the return value is. I guess it's the number of > bytes sent, or a network error code on failure. I updated the comment - meaningful return codes are < 0 meaning error or IO_PENDING and >= 0 meaning success. Requiring this class to return the total bytes sent would place a useless requirement on the class, since the class is responsible for ensuring that all the data is sent. It is an implementation artifact that the number of bytes sent (if only one write() was required) is returned. If you prefer, I can change all positive return values to OK. http://codereview.chromium.org/249031/diff/12001/5009#newcode45 Line 45: virtual int ReadResponseHeaders(CompletionCallback* callback) = 0; On 2009/10/09 02:22:39, wtc wrote: > Define the return value. Does it return the byte count or > OK on success? Done. http://codereview.chromium.org/249031/diff/12001/5009#newcode51 Line 51: // is returned, or an error is returned upon failure. ERR_CONNECTION_CLOSED On 2009/10/09 02:22:39, wtc wrote: > Can we avoid returning ERR_CONNECTION_CLOSED for EOF here? > This is a public method, In external interface of a class, > we should try to report EOF using the conventional 0 return > value. I'm confused by this request. Your own comment in HttpNetworkTransaction says, or at least implies that it would be better to return a status code like ERR_CONNECTION_CLOSED instead of returning 0. OK/0=EOF is ambiguous. Maybe this isn't the best time to make such a change, but it significantly simplifies handling of some error cases and makes the code easier to understand. I modified your initial comment and pushed it to all the places where callers receive 0 as EOF so that the rest of the stack can eventually get converted. http://codereview.chromium.org/249031/diff/12001/5009#newcode65 Line 65: // length. If neither is sent, the connection must close in order to On 2009/10/09 02:22:39, wtc wrote: > Nit: change > the connection must close ... > to > the server must close the connection for us to find the > end of the response. Done. http://codereview.chromium.org/249031/diff/12001/5009#newcode70 Line 70: // data has been read from the socket. On 2009/10/09 02:22:39, wtc wrote: > Add "(part of the next response)" after "data". Done. http://codereview.chromium.org/249031/diff/12001/5019 File net/http/http_stream_parser.cc (right): http://codereview.chromium.org/249031/diff/12001/5019#newcode78 Line 78: if (io_state_ == STATE_DONE) On 2009/10/09 02:22:39, wtc wrote: > Why don't you use the solution in > HttpNetworkTransaction::Read() and > HttpNetworkTransaction::DoReadBody()? > > Then we can get rid of the STATE_BODY_PENDING and STATE_DONE states. I need the state machine to transition through STATE_PROCESSING_BODY exactly once at the end of the response data in order to properly save any data that belongs to the next response. I could pull that code out to a function and call it from multiple places, but it might be more likely to break on modification. http://codereview.chromium.org/249031/diff/12001/5019#newcode127 Line 127: case STATE_REQUEST_SENT: On 2009/10/09 02:22:39, wtc wrote: > In the original http_network_transaction.cc code, we just > set next_state_ to STATE_NONE, which will cause us to break > out of the do-while loop. Are you sure you don't want to > do that? Here's my previous comment: > With STATE_NONE, you actually lose the state and depend on > the caller to callback into the state machine correctly. > In HttpNetworkTransaction, this only happens once - > transitioning from ReadHeadersComplete to ReadBody, so it > is less of an issue. But that transition is confusing > because it is unlike all the others. In HttpStream, there > are three points where the caller gets control back and > could call into HttpStream at the wrong place. > Furthermore, can_do_more and schedule_callback are more > explicit and make the state machine easier to follow. http://codereview.chromium.org/249031/diff/12001/5019#newcode157 Line 157: NOTREACHED(); On 2009/10/09 02:22:39, wtc wrote: > We need to set can_do_more to false, otherwise we will be > stuck in this loop. Done. http://codereview.chromium.org/249031/diff/12001/5019#newcode161 Line 161: On 2009/10/09 02:22:39, wtc wrote: > You always call ScheduleCallback() before you set > can_do_more to false. Indeed. This wasn't obvious from the state diagram I have drawn out, but it does seem to be true. This observation also shows that scheduled_callback_ is not needed. I've removed it. > This will require you to set result to OK in > STATE_REQUEST_SENT, STATE_BODY_PENDING, and > STATE_DONE. More precisely, it requires that result != ERR_IO_PENDING in those states, which is already true. I've added DCHECK's to make it make it easier to understand/ensure that it stays that way. http://codereview.chromium.org/249031/diff/12001/5019#newcode174 Line 174: result = connection_->socket()->Write(request_headers_, On 2009/10/09 02:22:39, wtc wrote: > If the Write() call returns a real error (not ERR_IO_PENDING), > you will go back to DoSendHeaders with a negative 'result'. > Then things will go wrong because you aren't checking if > result < 0. > This comment also applies to DoSendBody. There's: if (result < 0) can_do_more = false; before both DoSendHeaders and DoSendBody in DoLoop. I would prefer to do it generically in DoLoop, but some states need to be able to return an error. http://codereview.chromium.org/249031/diff/12001/5020 File net/http/http_stream_parser.h (right): http://codereview.chromium.org/249031/diff/12001/5020#newcode34 Line 34: // These functions implement the interface described in HttpStream with On 2009/10/09 02:22:39, wtc wrote: > Just wondering: why doesn't this class derive from > HttpStream then? It is not meant to be used as an HttpStream, and its interface is allowed to diverge from the HttpStream interface as it evolves. HttpStreamParser is a utility class that several HttpStream implementations can delegate work to. http://codereview.chromium.org/249031/diff/12001/5020#newcode61 Line 61: STATE_PROCESSING_HEADERS, On 2009/10/09 02:22:39, wtc wrote: > The renaming of the STATE_READ_HEADERS_COMPLETE to > STATE_PROCESSING_HEADERS and STATE_READ_BODY_COMPLETE > to STATE_PROCESSING_BODY seems gratuitous. It will > confuse people like me who are familiar with the original > code, with little gain. I find the original names confusing. STATE_READ_HEADERS_COMPLETE implies that the *headers* have been *completely* *read*. Transitioning back to STATE_READ_HEADERS is unintuitive. Reading & Processing more accurately describes the function of the states. I understand the consistency can win out over understandability, but the winner is unclear to me in this case, especially if I am allowed to keep the somewhat different state machine implementation. http://codereview.chromium.org/249031/diff/12001/5020#newcode137 Line 137: scoped_refptr<IOBuffer> body_buf_; On 2009/10/09 02:22:39, wtc wrote: > Nit: please rename these members response_body_buf_ > and response_body_buf_len_. Changed to an even more descriptive name: user_read_buf_
You may want to consider moving the following files to
a separate CL to make this CL look less daunting:
io_buffer.{h,cc}
http_chunked_decoder.{h,cc} and its unit test
http_response_info.{h,cc}
The reason I favor sticking to the original convention
for state machines, etc. is consistency. Here I quote
from our C++ style guide:
===
One way in which we keep the code base manageable is by enforcing consistency.
It is very important that any programmer be able to look at another's code and
quickly understand it. Maintaining a uniform style and following conventions
means that we can more easily use "pattern-matching" to infer what various
symbols are and what invariants are true about them. Creating common, required
idioms and patterns makes code much easier to understand. In some cases there
might be good arguments for changing certain style rules, but we nonetheless
keep things as they are in order to preserve consistency.
===
The "COMPLETE" in those STATE_FOO_COMPLETE states means
the potentially asynchronous operation FOO has completed.
It allows us to call the same function whether the operation
FOO completes synchronously or asynchronously. It's
unfortunate that "COMPLETE" is confusing when tagged
onto "READ_HEADERS" or "READ_BODY", but in exchange
we have a consistent pattern that helps people spot
functions that handle the completion of operations.
To use ERR_CONNECTION_CLOSED, we need to update its
comment in net_error_list.h and make sure no existing
code uses this error code. The following code from
tcp_client_socket_win.cc is where ERR_CONNECTION_CLOSED
was originally added:
case WSAEDISCON:
// Returned by WSARecv or WSARecvFrom for message-oriented sockets (where
// a return value of zero means a zero-byte message) to indicate graceful
// connection shutdown. We should not ever see this error code for TCP
// sockets, which are byte stream oriented.
NOTREACHED();
return ERR_CONNECTION_CLOSED;
Note the block comment and NOTREACHED() assertion.
In any case, it shows that originally ERR_CONNECTION_CLOSED
meant WSAEDISCON. If we change it to mean "end of
connection", we need to update existing code and comments.
Therefore, inventing a new error code such as ERR_EOF
(or some other name) could be a way to avoid the trouble.
Some of my comments may be incoherent because it's been
a while since I last thought through this issue.
http://codereview.chromium.org/249031/diff/12001/5021
File net/http/http_response_info.h (right):
http://codereview.chromium.org/249031/diff/12001/5021#newcode40
Line 40: // Used to correctly log the duration across authentication activities.
Please document when this member is set to true or false.
Nit: change
authentication activities
to
authentication restarts
to be more precise.
I separated the io buffer changes and the chunked encoder changes. Other comments addressed below. http://codereview.chromium.org/249031/diff/12001/5017 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/249031/diff/12001/5017#newcode797 Line 797: // TODO(wtc): Traditionally this code has returned 0 when reading a closed On 2009/10/09, wtc wrote: > To use ERR_CONNECTION_CLOSED, we need to update its > comment in net_error_list.h and make sure no existing > code uses this error code. The following code from > tcp_client_socket_win.cc is where ERR_CONNECTION_CLOSED > was originally added: greping for ERR_CONNECTION_CLOSED yields 10 uses in code. It looks like all but the one you referred to use it the way I use it. Specifically look at net/socket/socks_client_socket.cc. I not sure about the use in net/base/wininet_util.cc. Shall I submit a CL to change the one you referred to? http://codereview.chromium.org/249031/diff/12001/5021 File net/http/http_response_info.h (right): http://codereview.chromium.org/249031/diff/12001/5021#newcode40 Line 40: // Used to correctly log the duration across authentication activities. On 2009/10/09 21:41:26, wtc wrote: > Please document when this member is set to true or false. > > Nit: change > authentication activities > to > authentication restarts > to be more precise. Done. http://codereview.chromium.org/249031/diff/12001/5020 File net/http/http_stream_parser.h (right): http://codereview.chromium.org/249031/diff/12001/5020#newcode61 Line 61: STATE_PROCESSING_HEADERS, > On 2009/10/09, wtc wrote: > === > One way in which we keep the code base manageable is by > > enforcing consistency. > It is very important that any programmer be able to look at another's code and > quickly understand it. Maintaining a uniform style and following conventions > means that we can more easily use "pattern-matching" to infer what various > symbols are and what invariants are true about them. Creating common, required > idioms and patterns makes code much easier to understand. > In some cases there > might be good arguments for changing certain style rules, > but we nonetheless > keep things as they are in order to preserve consistency. > === > > The "COMPLETE" in those STATE_FOO_COMPLETE states means > the potentially asynchronous operation FOO has completed. > It allows us to call the same function whether the operation > FOO completes synchronously or asynchronously. It's > unfortunate that "COMPLETE" is confusing when tagged > onto "READ_HEADERS" or "READ_BODY", but in exchange > we have a consistent pattern that helps people spot > functions that handle the completion of operations. I maintain that the COMPLETE convention is confusing, but see that it is very common. I changed these two uses, though I added a comment that clarifies my initial confusion. Sorry, I should have searched how common this was before pushing back.
http://codereview.chromium.org/249031/diff/12001/5020 File net/http/http_stream_parser.h (right): http://codereview.chromium.org/249031/diff/12001/5020#newcode61 Line 61: STATE_PROCESSING_HEADERS, On 2009/10/09 23:54:01, vandebo wrote: > > I maintain that the COMPLETE convention is confusing, but see that it is very > common. I changed these two uses, though I added a comment that clarifies my > initial confusion. Sorry, I should have searched how common this was before > pushing back. You should not let me stifle improvements. What I insist on is consistency. How about this new convention? STATE_FOO STATE_PROCESS_FOO_RESULT (was: STATE_FOO_COMPLETE) Feel free to suggest other ideas. We can make this change in a follow-up CL because I want to change all the state machines in the net module.
LGTM. Sorry about the delay in reviewing this CL. Please check this in after making as many changes I suggested below as you can. I didn't have time to go through your response to my last review comments. So I may have suggested some changes that you already explained why you can't make. Sorry. http://codereview.chromium.org/249031/diff/20001/14005 File net/http/http_basic_stream.cc (right): http://codereview.chromium.org/249031/diff/20001/14005#newcode7 Line 7: #include "base/ref_counted.h" We shouldn't need to include ref_counted here because http_basic_stream.h already uses scoped_refptr. http://codereview.chromium.org/249031/diff/20001/14005#newcode9 Line 9: #include "net/http/http_chunked_decoder.h" Do we need to include http_chunked_decoder.h? http://codereview.chromium.org/249031/diff/20001/14005#newcode51 Line 51: return read_buf_->get_size() != read_buf_->get_offset(); This is the only place where this classes uses read_buf_. I suggest that we implement the IsMoreDataBuffered method in HttpStreamParser (it's declared but not defined), and then remove read_buf_ from this class and just call parser_->IsMoreDataBuffered(). http://codereview.chromium.org/249031/diff/20001/14008 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/249031/diff/20001/14008#newcode257 Line 257: // Even if they server says the connection is keep-alive, we have to be Typo: they => the http://codereview.chromium.org/249031/diff/20001/14008#newcode261 Line 261: // If there is a response body of known length, we need to drain it first. This comment should be updated. I guess you can change If there is a response body of known length to If the response body hasn't been completely read. With our code, http_stream_->IsResponseBodyComplete() should always return false at this point, if I'm not mistaken. http://codereview.chromium.org/249031/diff/20001/14008#newcode330 Line 330: scoped_refptr<HttpResponseHeaders> headers = GetResponseHeaders(); 'headers' is not used in this function. http://codereview.chromium.org/249031/diff/20001/14008#newcode433 Line 433: rv = DoSendRequest(); Can you make DoSendRequest take 'rv' as a dummy argument and restore the DCHECK_EQ(OK, rv) for consistency with the other states? http://codereview.chromium.org/249031/diff/20001/14008#newcode719 Line 719: if (establishing_tunnel_) Nit: please add braces {} around the if and else. http://codereview.chromium.org/249031/diff/20001/14008#newcode790 Line 790: if (response->first_response_time) { It seems better to make first_response_time a member of this class. You can add an "int restart_count" member to record the number of auth restarts. http://codereview.chromium.org/249031/diff/20001/14008#newcode800 Line 800: // socket. That is partially corrected below, but callers need to be It's not clear what you meant by "below" here. The code below isn't using 'result'. http://codereview.chromium.org/249031/diff/20001/14008#newcode896 Line 896: // Error while reading the socket. This comment (and the comment below at line 941) may need to be updated because ERR_CONNECTION_CLOSED isn't a real error. http://codereview.chromium.org/249031/diff/20001/14008#newcode899 Line 899: // socket. That is partially corrected below, but callers need to be Same here: not clear what you meant by "below". http://codereview.chromium.org/249031/diff/20001/14003 File net/http/http_network_transaction.h (right): http://codereview.chromium.org/249031/diff/20001/14003#newcode307 Line 307: bool headers_valid_; The comment isn't clear. It seems that you set headers_valid_ to true after you have read the response headers completely. It seems that this should be tracked by the HttpStream class. Nit: we try to declare all the bool members consecutively so that the compiler can pack them. You can see that in this class, except for ProxyMode proxy_mode_, which used to be a bool. http://codereview.chromium.org/249031/diff/20001/14011 File net/http/http_response_info.h (right): http://codereview.chromium.org/249031/diff/20001/14011#newcode41 Line 41: // Set in http parsing code and used in HttpNetworkTransaction where It's also reset by HttpNetworkTransaction. I think first_response_time should be moved to HttpNetworkTransaction. http://codereview.chromium.org/249031/diff/20001/14002 File net/http/http_stream.h (right): http://codereview.chromium.org/249031/diff/20001/14002#newcode33 Line 33: // when available. Non-negative values indicates success. The HttpStream Nit: indicates => indicate If non-negative values are actually byte counts, please say so. Note: since UploadDataStream could include a large file (whose size is larger than 2GB), 'int' won't be able to represent an upload byte count in general, so if the byte count is not useful, we should just return OK on success. We can return the byte count in an 'int64' (or 'uint64'?) output variable if the byte count is useful. http://codereview.chromium.org/249031/diff/20001/14002#newcode36 Line 36: const std::string& headers, Nit: for consistency, name these two parameters request_headers request_body or just headers body Here the context makes it clear that 'headers' and 'body' belong to the request. http://codereview.chromium.org/249031/diff/20001/14002#newcode46 Line 46: // to the callback when available. Non-negative values indicate success. Same comment here: do non-negative values represent byte counts? Might want to document where the response headers go to, since this function doesn't have an output parameter. http://codereview.chromium.org/249031/diff/20001/14002#newcode54 Line 54: // is returned to indicate end-of-file. ERR_IO_PENDING is returned if the Let's change "end-of-file" to "end of connection" here. http://codereview.chromium.org/249031/diff/20001/14009 File net/http/http_stream_parser.cc (right): http://codereview.chromium.org/249031/diff/20001/14009#newcode103 Line 103: int HttpStreamParser::DoLoop(int result) { Some comments about this DoLoop: 1. You don't set io_state_ to STATE_NONE before entering the switch statement. This differs from the original state machine loop. I found that most people (including me) seem to prefer your new way, but now we are losing a common way of implementing state machine loops. 2. The reason you need the if (result < 0) in the STATE_SENDING_HEADERS and STATE_SENDING_BODY states is that you don't haven't the corresponding _COMPLETE states. I suggest that you add the corresponding _COMPLETE states and DoFooComplete functions. 3. You don't really need the STATE_REQUEST_SENT state. It's only used in a DCHECK in ReadResponseHeaders. Similarly for STATE_BODY_PENDING, which is only used in a DCHECK in ReadResponseBody. 4. You can replace can_do_more by io_state != the terminal states, which are STATE_REQUEST_SENT, STATE_BODY_PENDING, and STATE_DONE. In the original state machine loop, we use STATE_NONE to break out of the state loop, and use class members to remember where we are. I think your new way could lead to simplifications of some code because we only need to check the io_state_ member to know where we are in the lifetime of this object. http://codereview.chromium.org/249031/diff/20001/14009#newcode226 Line 226: connection_->should_resend_failed_request(result)) { The indentation of this line seems to be off by two spaces. http://codereview.chromium.org/249031/diff/20001/14009#newcode464 Line 464: "chunked")) { Nit: align this with the "Transfer-Encoding" argument on the previous line. http://codereview.chromium.org/249031/diff/20001/14009#newcode488 Line 488: else if (response_body_length_ != -1) Nit: don't use "else" after a return statement. http://codereview.chromium.org/249031/diff/20001/14010 File net/http/http_stream_parser.h (right): http://codereview.chromium.org/249031/diff/20001/14010#newcode29 Line 29: // with |read_buffer,| data will be moved so that offset is zero again. Nit: |read_buffer,| => |read_buffer|, http://codereview.chromium.org/249031/diff/20001/14010#newcode57 Line 57: enum IOState { I suggest that you name this type 'State' and the variable 'next_state_' for consistency with other state machines in net. http://codereview.chromium.org/249031/diff/20001/14010#newcode83 Line 83: // Try to make progress sending/receiving the request. Nit: receiving the *response*. http://codereview.chromium.org/249031/diff/20001/14013 File net/socket/client_socket_handle.h (right): http://codereview.chromium.org/249031/diff/20001/14013#newcode107 Line 107: bool should_resend_failed_request(int error) const { This method does not belong here. It should be in HttpNetworkTransaction. This function uses only the public method reuse_type() and public type SocketReuseType, so it can be easily moved to another class. Also, an accessor-like name (should_resend_failed_request) should only be used when the function is as cheap as an accessor. This function is rather complicated, so it should have the normal function name ShouldResendFailedRequest.
wtc, thank you for all the comments. I know this is a difficult CL to look at. If there is anything that I haven't addressed that you feel strongly about, please say so. There is one outstanding issue you raised that I hope you can comment on a bit more. The issue of ERR_CONNECTION_CLOSED. You said: > To use ERR_CONNECTION_CLOSED, we need to update its > comment in net_error_list.h and make sure no existing > code uses this error code. The following code from > tcp_client_socket_win.cc is where ERR_CONNECTION_CLOSED > was originally added: greping for ERR_CONNECTION_CLOSED yields 10 uses in code. It looks like all but the one you referred to use it the way I use it. Though I'm not sure about the use in net/base/wininet_util.cc. What needs to be done to make this error code consistent? http://codereview.chromium.org/249031/diff/20001/14005 File net/http/http_basic_stream.cc (right): http://codereview.chromium.org/249031/diff/20001/14005#newcode7 Line 7: #include "base/ref_counted.h" On 2009/10/13 22:47:36, wtc wrote: > We shouldn't need to include ref_counted here because > http_basic_stream.h already uses scoped_refptr. Done. http://codereview.chromium.org/249031/diff/20001/14005#newcode9 Line 9: #include "net/http/http_chunked_decoder.h" On 2009/10/13 22:47:36, wtc wrote: > Do we need to include http_chunked_decoder.h? Done. http://codereview.chromium.org/249031/diff/20001/14005#newcode51 Line 51: return read_buf_->get_size() != read_buf_->get_offset(); On 2009/10/13 22:47:36, wtc wrote: > This is the only place where this classes uses read_buf_. > > I suggest that we implement the IsMoreDataBuffered method > in HttpStreamParser (it's declared but not defined), and > then remove read_buf_ from this class and just call > parser_->IsMoreDataBuffered(). For some IOBuffer review changes I made, this calls the now implemented http_stream method. But I want to keep read_buf_ here, as the HttpStreamParser interface is part of my plan to actually get to pipelining. http://codereview.chromium.org/249031/diff/20001/14008 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/249031/diff/20001/14008#newcode257 Line 257: // Even if they server says the connection is keep-alive, we have to be On 2009/10/13 22:47:36, wtc wrote: > Typo: they => the Done. http://codereview.chromium.org/249031/diff/20001/14008#newcode261 Line 261: // If there is a response body of known length, we need to drain it first. On 2009/10/13 22:47:36, wtc wrote: > This comment should be updated. I guess you can change > If there is a response body of known length > to > If the response body hasn't been completely read. Done. > With our code, http_stream_->IsResponseBodyComplete() > should always return false at this point, if I'm not > mistaken. I'm not sure about that. http://codereview.chromium.org/249031/diff/20001/14008#newcode330 Line 330: scoped_refptr<HttpResponseHeaders> headers = GetResponseHeaders(); On 2009/10/13 22:47:36, wtc wrote: > 'headers' is not used in this function. Done. http://codereview.chromium.org/249031/diff/20001/14008#newcode433 Line 433: rv = DoSendRequest(); On 2009/10/13 22:47:36, wtc wrote: > Can you make DoSendRequest take 'rv' as a dummy argument > and restore the DCHECK_EQ(OK, rv) for consistency with the > other states? Added the missing check. The arguments are consistent with the other state functions already. http://codereview.chromium.org/249031/diff/20001/14008#newcode719 Line 719: if (establishing_tunnel_) On 2009/10/13 22:47:36, wtc wrote: > Nit: please add braces {} around the if and else. Done. http://codereview.chromium.org/249031/diff/20001/14008#newcode790 Line 790: if (response->first_response_time) { On 2009/10/13 22:47:36, wtc wrote: > It seems better to make first_response_time a member of this > class. You can add an "int restart_count" member to record > the number of auth restarts. Indeed. Done. http://codereview.chromium.org/249031/diff/20001/14008#newcode800 Line 800: // socket. That is partially corrected below, but callers need to be On 2009/10/13 22:47:36, wtc wrote: > It's not clear what you meant by "below" here. The code > below isn't using 'result'. Done. http://codereview.chromium.org/249031/diff/20001/14008#newcode896 Line 896: // Error while reading the socket. On 2009/10/13 22:47:36, wtc wrote: > This comment (and the comment below at line 941) may need > to be updated because ERR_CONNECTION_CLOSED isn't a real > error. Done. http://codereview.chromium.org/249031/diff/20001/14008#newcode899 Line 899: // socket. That is partially corrected below, but callers need to be On 2009/10/13 22:47:36, wtc wrote: > Same here: not clear what you meant by "below". Done. http://codereview.chromium.org/249031/diff/20001/14003 File net/http/http_network_transaction.h (right): http://codereview.chromium.org/249031/diff/20001/14003#newcode307 Line 307: bool headers_valid_; On 2009/10/13 22:47:36, wtc wrote: > The comment isn't clear. Done. > It seems that you set headers_valid_ to true after you > have read the > response headers completely. It seems that this should > be tracked by the HttpStream class. In order to not change the semantics of HttpNetworkTransaction::GetResponseInfo(), this can only be set once the headers are valid as far as HttpNetworkTransaction is concerned. i.e. a 1xx response in HttpStreamParser gives a valid header, but not in HttpNetworkTransaction. > Nit: we try to declare all the bool members consecutively > so that the compiler can pack them. Done. http://codereview.chromium.org/249031/diff/20001/14011 File net/http/http_response_info.h (right): http://codereview.chromium.org/249031/diff/20001/14011#newcode41 Line 41: // Set in http parsing code and used in HttpNetworkTransaction where On 2009/10/13 22:47:36, wtc wrote: > It's also reset by HttpNetworkTransaction. > I think first_response_time should be moved to > HttpNetworkTransaction. Done. http://codereview.chromium.org/249031/diff/20001/14002 File net/http/http_stream.h (right): http://codereview.chromium.org/249031/diff/20001/14002#newcode33 Line 33: // when available. Non-negative values indicates success. The HttpStream On 2009/10/13 22:47:36, wtc wrote: > Nit: indicates => indicate > > If non-negative values are actually byte counts, please say > so. Note: since UploadDataStream could include a large > file (whose size is larger than 2GB), 'int' won't be able > to represent an upload byte count in general, so if the > byte count is not useful, we should just return OK on success. We can return > the byte count in an 'int64' > (or 'uint64'?) output variable if the byte count is useful. Done. http://codereview.chromium.org/249031/diff/20001/14002#newcode36 Line 36: const std::string& headers, On 2009/10/13 22:47:36, wtc wrote: > Nit: for consistency, name these two parameters > request_headers > request_body > or just > headers > body > > Here the context makes it clear that 'headers' and 'body' > belong to the request. Done. http://codereview.chromium.org/249031/diff/20001/14002#newcode46 Line 46: // to the callback when available. Non-negative values indicate success. On 2009/10/13 22:47:36, wtc wrote: > Same comment here: do non-negative values represent byte > counts? > > Might want to document where the response headers go to, > since this function doesn't have an output parameter. Done. http://codereview.chromium.org/249031/diff/20001/14002#newcode54 Line 54: // is returned to indicate end-of-file. ERR_IO_PENDING is returned if the On 2009/10/13 22:47:36, wtc wrote: > Let's change "end-of-file" to "end of connection" here. Done. http://codereview.chromium.org/249031/diff/20001/14009 File net/http/http_stream_parser.cc (right): http://codereview.chromium.org/249031/diff/20001/14009#newcode103 Line 103: int HttpStreamParser::DoLoop(int result) { On 2009/10/13 22:47:36, wtc wrote: > Some comments about this DoLoop: > > 1. You don't set io_state_ to STATE_NONE before entering > the switch statement. This differs from the original > state machine loop. I found that most people (including me) > seem to prefer your new way, but now we are losing a > common way of implementing state machine loops. I think this way is easier to understand enough that it's worth breaking away from commonality. To me, the purpose of including an explicit state machine (most of the state machines could be replaced with a function pointer to hold maintain state across IO's) is to make it clear and explicit what the current state of things is, by transitioning in and out of STATE_NONE, you lose a lot of that. > 2. The reason you need the if (result < 0) in the > STATE_SENDING_HEADERS and STATE_SENDING_BODY states is that > you don't haven't the corresponding _COMPLETE states. > I suggest that you add the corresponding _COMPLETE states > and DoFooComplete functions. Adding _COMPLETE states won't help. HttpNetworkTransaction::DoLoop handles error conditions by leaving next_state == STATE_NONE. > 3. You don't really need the STATE_REQUEST_SENT state. > It's only used in a DCHECK in ReadResponseHeaders. > Similarly for STATE_BODY_PENDING, which is only used in a > DCHECK in ReadResponseBody. As you point out in your next point, each of these states is a terminal state, that should exit the state machine until the class in called back into. I can't think of a reasonable way to implement that without introducing these states. > 4. You can replace can_do_more by io_state != the terminal > states, which are STATE_REQUEST_SENT, STATE_BODY_PENDING, and STATE_DONE. In > the original state machine loop, we > use STATE_NONE to break out of the state loop, and use > class members to remember where we are. I think your new > way could lead to simplifications of some code because > we only need to check the io_state_ member to know where > we are in the lifetime of this object. can_do_more = false is also used to break out of the loop for exceptional conditions, so just checking the terminal states won't work. http://codereview.chromium.org/249031/diff/20001/14009#newcode226 Line 226: connection_->should_resend_failed_request(result)) { On 2009/10/13 22:47:36, wtc wrote: > The indentation of this line seems to be off by two spaces. Done. http://codereview.chromium.org/249031/diff/20001/14009#newcode464 Line 464: "chunked")) { On 2009/10/13 22:47:36, wtc wrote: > Nit: align this with the "Transfer-Encoding" argument on > the previous line. Done. http://codereview.chromium.org/249031/diff/20001/14009#newcode488 Line 488: else if (response_body_length_ != -1) On 2009/10/13 22:47:36, wtc wrote: > Nit: don't use "else" after a return statement. I can understand not using else return foo; But if (foo) return x; else if (bar) return y; is different - it provides information to future code readers about the relationship of the two conditionals - that the order you check them in is important. http://codereview.chromium.org/249031/diff/20001/14010 File net/http/http_stream_parser.h (right): http://codereview.chromium.org/249031/diff/20001/14010#newcode29 Line 29: // with |read_buffer,| data will be moved so that offset is zero again. On 2009/10/13 22:47:36, wtc wrote: > Nit: |read_buffer,| => |read_buffer|, Done. http://codereview.chromium.org/249031/diff/20001/14010#newcode57 Line 57: enum IOState { On 2009/10/13 22:47:36, wtc wrote: > I suggest that you name this type 'State' and the variable > 'next_state_' for consistency with other state machines in > net. Enum changed. io_state_ is not used the same way as next_state_, so naming it that would be confusing. http://codereview.chromium.org/249031/diff/20001/14010#newcode83 Line 83: // Try to make progress sending/receiving the request. On 2009/10/13 22:47:36, wtc wrote: > Nit: receiving the *response*. Done. http://codereview.chromium.org/249031/diff/20001/14013 File net/socket/client_socket_handle.h (right): http://codereview.chromium.org/249031/diff/20001/14013#newcode107 Line 107: bool should_resend_failed_request(int error) const { On 2009/10/13 22:47:36, wtc wrote: > This method does not belong here. It should be in > HttpNetworkTransaction. This function uses only the > public method reuse_type() and public type SocketReuseType, > so it can be easily moved to another class. As previously mentioned this is the common ancestor of the two places where this function is needed. > Also, an accessor-like name (should_resend_failed_request) > should only be used when the function is as cheap as an > accessor. This function is rather complicated, so it > should have the normal function name ShouldResendFailedRequest. Done.
Steve, seems like you're waiting for my reply. Sorry about that. I've been very busy and won't have time this week to look at this CL again. Please check in this CL. I'll file a bug to deal with ERR_CONNECTION_CLOSED.
LGTM. Please check this in after fixing the nits below without further review from me. I reviewed the delta between Patch Sets 10 and 11. There are a lot of changes to http_stream_parser.cc, which I just skimmed through. (It's hard to review a large CL carefully several times. Sorry!) Re: ERR_CONNECTION_CLOSED: I filed http://crbug.com/25032 on this issue. The existing usage of ERR_CONNECTION_CLOSED in SOCKS client sockets is for a real error -- it means the underlying connection (below the SOCKS layer) was closed unexpectedly. It's a graceful connection closure in the lower layer, but an abrupt connection closure in the SOCKS layer, hence an error. That is different from the meaning of ERR_CONNECTION_CLOSED in this CL. Let's follow up on ERR_CONNECTION_CLOSED in http://crbug.com/25032. http://codereview.chromium.org/249031/diff/20001/14008 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/249031/diff/20001/14008#newcode261 Line 261: // If there is a response body of known length, we need to drain it first. On 2009/10/15 04:54:02, vandebo wrote: > > > With our code, http_stream_->IsResponseBodyComplete() > > should always return false at this point, if I'm not > > mistaken. > > I'm not sure about that. This is because our code doesn't read the response body after getting a 401 or 407 response; we immediately start to handle the authentication challenge. The response body is left unread. So unless the response body length is 0, http_stream_->IsResponseBodyComplete() should return false here. In any case, this is a minor issue, and doing the !http_stream_->IsResponseBodyComplete() test here is harmless even if it's unnecessary. http://codereview.chromium.org/249031/diff/20001/14009 File net/http/http_stream_parser.cc (right): http://codereview.chromium.org/249031/diff/20001/14009#newcode488 Line 488: else if (response_body_length_ != -1) On 2009/10/15 04:54:02, vandebo wrote: > > I can understand not using else return foo; But > > if (foo) > return x; > else if (bar) > return y; > > is different - it provides information to future code readers about the > relationship of the two conditionals - that the order you check them in is > important. You can write that like this: if (foo) return x; if (bar) return y; http://codereview.chromium.org/249031/diff/18010/18011 File net/http/http_stream.h (right): http://codereview.chromium.org/249031/diff/18010/18011#newcode47 Line 47: // headers are filled into the HttpRequestInfo passed into SendRequest. This comment is wrong. SendRequest takes a const HttpRequestInfo* request argument. How do we fill responder headers into a const HttpRequestInfo? Also, HttpRequestInfo doesn't have a response headers field. I think this comment should read: The response headers are filled into the HttpResponseInfo returned by GetResponseInfo. Or just delete this comment. http://codereview.chromium.org/249031/diff/18010/18017 File net/http/http_stream_parser.cc (right): http://codereview.chromium.org/249031/diff/18010/18017#newcode58 Line 58: return result; Nit: you can also just do return result > 0 : OK : result; We use that expression elsewhere. I leave this up to you... http://codereview.chromium.org/249031/diff/18010/18017#newcode367 Line 367: // some left over in |user_read_buf|, plus there may be more Typo: user_read_buf => user_read_buf_ user_buf => user_buf_ Two occurrences of each in this comment block. http://codereview.chromium.org/249031/diff/18010/18018 File net/http/http_stream_parser.h (right): http://codereview.chromium.org/249031/diff/18010/18018#newcode26 Line 26: // |read_buffer|. The offset in |read_buffer| indicates valid data. "The offset in |read_buffer| indicates valid data." Does this refer to the offset before or after parsing the stream? Does the offset indicates the beginning of the end of valid data? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
