Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10389007:
Change TCPListenSocket::SendInternal to use a non-blocking implementation. (Closed)
|
Created:
8 years, 7 months ago by Marshall Modified:
8 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionChange TCPListenSocket::SendInternal to use a non-blocking implementation.
HttpServer uses TCPListenSocket to create a server that runs on the browser process IO thread. The IO thread should not be blocked as this can introduce jank and deadlocks.
BUG=125191
TEST=TCPListenSocketTest.*, DevTools works (see bug report)
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=136572
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 26
Patch Set 4 : #
Total comments: 6
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 18
Patch Set 8 : #
Total comments: 12
Patch Set 9 : #
Messages
Total messages: 32 (0 generated)
Please review. willchan@ = OWNER michaeln@, pfeldman@ = FYI
Additionally, this patch set fixes a problem on Windows where running: net_unittests.exe --gtest_filter=TCPListenSocketTest.* fails because Winsock is not initialized.
I'm at a conference. Adding mmenke to review this since he's reviewing other stuff here too anyway. On Mon, May 7, 2012 at 1:23 PM, <marshall@chromium.org> wrote: > Additionally, this patch set fixes a problem on Windows where running: > > net_unittests.exe --gtest_filter=**TCPListenSocketTest.* > > fails because Winsock is not initialized. > > http://codereview.chromium.**org/10389007/<http://codereview.chromium.org/103... >
The unit tests seem to assume that a single call to SendInternal always sends everything in the string. This may be a reasonable assumption, given that TCPListenSocketTester::Send apparently works, but I'd rather not rely on it. I'd also like to see a test that is likely to trigger the new code, if we can manage one. A test that sends a large amount of data at once may be able to achieve this. http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.cc File net/base/tcp_listen_socket.cc (right): http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:177: send_data_.append(bytes + sent, len - sent); nit: Since send_data_ is empty, suggest you just use .assign here, and below. http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:188: static_cast<const char*>(NULL), 0)); I think you should at least use a different function for the timer, to simplify the if statements and class invariants. As things are, the two uses of this function make it pretty ugly. http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:191: } I wonder if we really need two copies of the send code. Since we already do a couple rather horrible things, performance wise (polling, the substr call), I wonder if we wouldn't just be better off using a single copy of the send code, and basically get rid of the block of code above, except the initial DCHECKs. http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:225: if (!has_pending_writes_) { May be a little simpler to use a OneShotTimer, rather than the weak_ptr_ / has_pending_writes_ combination, though think it's fine as-is. http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket_... File net/base/tcp_listen_socket_unittest.cc (right): http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket_... net/base/tcp_listen_socket_unittest.cc:122: EXPECT_TRUE(server_); If you do ASSERT_TRUE(server_), you can remove the if below.
http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.cc File net/base/tcp_listen_socket.cc (right): http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:222: send_data_ = send_data_.substr(sent, len_left - sent); This is so ugly...you may want to use a scoped_array of DrainableIOBuffers or something.
https://chromiumcodereview.appspot.com/10389007/diff/8001/net/base/tcp_listen... File net/base/tcp_listen_socket.cc (right): https://chromiumcodereview.appspot.com/10389007/diff/8001/net/base/tcp_listen... net/base/tcp_listen_socket.cc:142: void TCPListenSocket::SendInternal(const char* bytes, int len) { Are there any consumers of this class that could be adversely affected by this change in behavior? Not saying there is, just asking. https://chromiumcodereview.appspot.com/10389007/diff/8001/net/base/tcp_listen... net/base/tcp_listen_socket.cc:186: MessageLoop::current()->PostTask(FROM_HERE, I would expect this to spin pretty hard when network backs up, retrying repeatedly at a very high rate waiting on the comparatively slow network to make progress. There probably need to be some delays in here alleviate that. Ideally, this would not employ polling at all and instead listen for notifications of previous writes completing so that new data may be written. Given the narrowness of the use case, polling at a more reasonable rate maybe with some backoff seems like an OK solution for now. https://chromiumcodereview.appspot.com/10389007/diff/8001/net/base/tcp_listen... net/base/tcp_listen_socket.cc:195: send_data_.append(bytes, len); What if no progress is made for a very long time, how large should this buffer be allowed to grow prior to giving up, and how long till we give up? And when giving up, what does that look like? There is no error return value from SendInternal and the family of Send methods. Looks like this class needs care-and-feeding beyond the scope of just this CL. For just this CL, I think you should make decisions about "how large" and "how long" that work for your immediate use case and call it a day. And once the class has "given up", dont try to send anymore after that for the life of the connection (to start sending again would be bad because data would have been lost and any data sent to the receiver beyond what was lost would look like corruption).
http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.cc File net/base/tcp_listen_socket.cc (right): http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:142: void TCPListenSocket::SendInternal(const char* bytes, int len) { On 2012/05/08 18:17:12, michaeln wrote: > Are there any consumers of this class that could be adversely affected by this > change in behavior? Not saying there is, just asking. From what I can tell TCPListenSocket is only used by HttpServer and unit tests (tcp_listen_socket_unittest.cc. transport_client_socket_unittest.cc). HttpServer is only used by devtools_http_handler_impl.cc. http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:177: send_data_.append(bytes + sent, len - sent); On 2012/05/08 15:51:57, Matt Menke wrote: > nit: Since send_data_ is empty, suggest you just use .assign here, and below. Done. http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:186: MessageLoop::current()->PostTask(FROM_HERE, On 2012/05/08 18:17:12, michaeln wrote: > I would expect this to spin pretty hard when network backs up, retrying > repeatedly at a very high rate waiting on the comparatively slow network to make > progress. There probably need to be some delays in here alleviate that. > > Ideally, this would not employ polling at all and instead listen for > notifications of previous writes completing so that new data may be written. > Given the narrowness of the use case, polling at a more reasonable rate maybe > with some backoff seems like an OK solution for now. I've implemented an exponential back-off using net::BackoffEntry. If a send call blocks it will delay the next send call by a minimum of 100ms up to a maximum of 500ms if the calls continue to block. If the buffer is flushed completely the delay will be reset to 0ms. If partial data is sent the back-off will be reduced. http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:188: static_cast<const char*>(NULL), 0)); On 2012/05/08 15:51:57, Matt Menke wrote: > I think you should at least use a different function for the timer, to simplify > the if statements and class invariants. As things are, the two uses of this > function make it pretty ugly. Done. http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:191: } On 2012/05/08 15:51:57, Matt Menke wrote: > I wonder if we really need two copies of the send code. Since we already do a > couple rather horrible things, performance wise (polling, the substr call), I > wonder if we wouldn't just be better off using a single copy of the send code, > and basically get rid of the block of code above, except the initial DCHECKs. Done. http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:195: send_data_.append(bytes, len); On 2012/05/08 18:17:12, michaeln wrote: > What if no progress is made for a very long time, how large should this buffer > be allowed to grow prior to giving up, and how long till we give up? And when > giving up, what does that look like? There is no error return value from > SendInternal and the family of Send methods. > > Looks like this class needs care-and-feeding beyond the scope of just this CL. > For just this CL, I think you should make decisions about "how large" and "how > long" that work for your immediate use case and call it a day. And once the > class has "given up", dont try to send anymore after that for the life of the > connection (to start sending again would be bad because data would have been > lost and any data sent to the receiver beyond what was lost would look like > corruption). I've implemented a maximum buffer size of 5MB. In my testing the buffer tends to reach a max size of between 1MB and 2MB, so 5MB seems like a safe margin. Currently it will output LOG(ERROR) and refuse to send additional data if this limit is reached. I guess we could take this a step further by closing the socket connection and/or asserting in the code. I agree that more work is needed on this class in the future. http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:222: send_data_ = send_data_.substr(sent, len_left - sent); On 2012/05/08 17:08:46, Matt Menke wrote: > This is so ugly...you may want to use a scoped_array of DrainableIOBuffers or > something. The buffer can grow to > 1MB in size. Do we want to keep a dedicated multi-MB buffer for TCPListenSocket? If so, a circular buffer implementation may be the best option. What do you think? http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:225: if (!has_pending_writes_) { On 2012/05/08 15:51:57, Matt Menke wrote: > May be a little simpler to use a OneShotTimer, rather than the weak_ptr_ / > has_pending_writes_ combination, though think it's fine as-is. Done. http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket_... File net/base/tcp_listen_socket_unittest.cc (right): http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket_... net/base/tcp_listen_socket_unittest.cc:122: EXPECT_TRUE(server_); On 2012/05/08 15:51:57, Matt Menke wrote: > If you do ASSERT_TRUE(server_), you can remove the if below. Done.
On 2012/05/08 15:51:57, Matt Menke wrote: > The unit tests seem to assume that a single call to SendInternal always sends > everything in the string. This may be a reasonable assumption, given that > TCPListenSocketTester::Send apparently works, but I'd rather not rely on it. > > I'd also like to see a test that is likely to trigger the new code, if we can > manage one. A test that sends a large amount of data at once may be able to > achieve this. I'll see if I can develop a test for this. I may have to re-factor the existing unit test implementations because they make blocking assumptions.
http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.cc File net/base/tcp_listen_socket.cc (right): http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:142: void TCPListenSocket::SendInternal(const char* bytes, int len) { On 2012/05/08 20:27:07, Marshall wrote: > On 2012/05/08 18:17:12, michaeln wrote: > > Are there any consumers of this class that could be adversely affected by this > > change in behavior? Not saying there is, just asking. > > From what I can tell TCPListenSocket is only used by HttpServer and unit tests > (tcp_listen_socket_unittest.cc. transport_client_socket_unittest.cc). HttpServer > is only used by devtools_http_handler_impl.cc. It's also used by chrome_frame/test/test_server http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:195: send_data_.append(bytes, len); On 2012/05/08 20:27:07, Marshall wrote: > On 2012/05/08 18:17:12, michaeln wrote: > > What if no progress is made for a very long time, how large should this buffer > > be allowed to grow prior to giving up, and how long till we give up? And when > > giving up, what does that look like? There is no error return value from > > SendInternal and the family of Send methods. > > > > Looks like this class needs care-and-feeding beyond the scope of just this CL. > > For just this CL, I think you should make decisions about "how large" and "how > > long" that work for your immediate use case and call it a day. And once the > > class has "given up", dont try to send anymore after that for the life of the > > connection (to start sending again would be bad because data would have been > > lost and any data sent to the receiver beyond what was lost would look like > > corruption). > > I've implemented a maximum buffer size of 5MB. In my testing the buffer tends to > reach a max size of between 1MB and 2MB, so 5MB seems like a safe margin. > Currently it will output LOG(ERROR) and refuse to send additional data if this > limit is reached. I guess we could take this a step further by closing the > socket connection and/or asserting in the code. > > I agree that more work is needed on this class in the future. In the medium to long term, we do intend to eventually get rid of this class in favor of ServerSocket, so there is the question of just how much effort should be spent on cleaning up this class. http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:195: send_data_.append(bytes, len); On 2012/05/08 20:27:07, Marshall wrote: > On 2012/05/08 18:17:12, michaeln wrote: > > What if no progress is made for a very long time, how large should this buffer > > be allowed to grow prior to giving up, and how long till we give up? And when > > giving up, what does that look like? There is no error return value from > > SendInternal and the family of Send methods. > > > > Looks like this class needs care-and-feeding beyond the scope of just this CL. > > For just this CL, I think you should make decisions about "how large" and "how > > long" that work for your immediate use case and call it a day. And once the > > class has "given up", dont try to send anymore after that for the life of the > > connection (to start sending again would be bad because data would have been > > lost and any data sent to the receiver beyond what was lost would look like > > corruption). > > I've implemented a maximum buffer size of 5MB. In my testing the buffer tends to > reach a max size of between 1MB and 2MB, so 5MB seems like a safe margin. > Currently it will output LOG(ERROR) and refuse to send additional data if this > limit is reached. I guess we could take this a step further by closing the > socket connection and/or asserting in the code. > > I agree that more work is needed on this class in the future. "Tends to?" How strong of an assurance do we have of that? I really should take a look at consumers of this class before signing off on anything. http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:222: send_data_ = send_data_.substr(sent, len_left - sent); On 2012/05/08 20:27:07, Marshall wrote: > On 2012/05/08 17:08:46, Matt Menke wrote: > > This is so ugly...you may want to use a scoped_array of DrainableIOBuffers or > > something. > > The buffer can grow to > 1MB in size. Do we want to keep a dedicated multi-MB > buffer for TCPListenSocket? If so, a circular buffer implementation may be the > best option. What do you think? I think that's reasonable. The one problem with a ringbuffer is resizing one is a little tricky, but really shouldn't be too bad at all. Probably a better solution than my suggestion, was just trying to keep things simple. http://codereview.chromium.org/10389007/diff/5/net/base/tcp_listen_socket.cc File net/base/tcp_listen_socket.cc (right): http://codereview.chromium.org/10389007/diff/5/net/base/tcp_listen_socket.cc#... net/base/tcp_listen_socket.cc:179: } else if (send_data_.size() > kMaxSendBufSize) { Should this be send_data_.size() + len? We'd still be allowing the first send to exceed the limit. http://codereview.chromium.org/10389007/diff/5/net/base/tcp_listen_socket.cc#... net/base/tcp_listen_socket.cc:182: send_data_error_ = true; send_data_.clear()?
http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.cc File net/base/tcp_listen_socket.cc (right): http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:195: send_data_.append(bytes, len); On 2012/05/08 20:51:43, Matt Menke wrote: > On 2012/05/08 20:27:07, Marshall wrote: > > On 2012/05/08 18:17:12, michaeln wrote: > > > What if no progress is made for a very long time, how large should this > buffer > > > be allowed to grow prior to giving up, and how long till we give up? And > when > > > giving up, what does that look like? There is no error return value from > > > SendInternal and the family of Send methods. > > > > > > Looks like this class needs care-and-feeding beyond the scope of just this > CL. > > > For just this CL, I think you should make decisions about "how large" and > "how > > > long" that work for your immediate use case and call it a day. And once the > > > class has "given up", dont try to send anymore after that for the life of > the > > > connection (to start sending again would be bad because data would have been > > > lost and any data sent to the receiver beyond what was lost would look like > > > corruption). > > > > I've implemented a maximum buffer size of 5MB. In my testing the buffer tends > to > > reach a max size of between 1MB and 2MB, so 5MB seems like a safe margin. > > Currently it will output LOG(ERROR) and refuse to send additional data if this > > limit is reached. I guess we could take this a step further by closing the > > socket connection and/or asserting in the code. > > > > I agree that more work is needed on this class in the future. > > "Tends to?" How strong of an assurance do we have of that? I really should > take a look at consumers of this class before signing off on anything. Not a very strong assurance at all, at least with DevTools. For example, it may be sending a large HTML page at the same time that the renderer is blocked on a sync message. In that case a significant portion of the HTML page may end up in the buffer. http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:222: send_data_ = send_data_.substr(sent, len_left - sent); On 2012/05/08 20:51:43, Matt Menke wrote: > On 2012/05/08 20:27:07, Marshall wrote: > > On 2012/05/08 17:08:46, Matt Menke wrote: > > > This is so ugly...you may want to use a scoped_array of DrainableIOBuffers > or > > > something. > > > > The buffer can grow to > 1MB in size. Do we want to keep a dedicated multi-MB > > buffer for TCPListenSocket? If so, a circular buffer implementation may be the > > best option. What do you think? > > I think that's reasonable. The one problem with a ringbuffer is resizing one is > a little tricky, but really shouldn't be too bad at all. Probably a better > solution than my suggestion, was just trying to keep things simple. Are there any currently existing ring buffer implementations that can be used from net/base?
http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.cc File net/base/tcp_listen_socket.cc (right): http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:222: send_data_ = send_data_.substr(sent, len_left - sent); On 2012/05/08 21:04:57, Marshall wrote: > On 2012/05/08 20:51:43, Matt Menke wrote: > > On 2012/05/08 20:27:07, Marshall wrote: > > > On 2012/05/08 17:08:46, Matt Menke wrote: > > > > This is so ugly...you may want to use a scoped_array of DrainableIOBuffers > > or > > > > something. > > > > > > The buffer can grow to > 1MB in size. Do we want to keep a dedicated > multi-MB > > > buffer for TCPListenSocket? If so, a circular buffer implementation may be > the > > > best option. What do you think? > > > > I think that's reasonable. The one problem with a ringbuffer is resizing one > is > > a little tricky, but really shouldn't be too bad at all. Probably a better > > solution than my suggestion, was just trying to keep things simple. > > Are there any currently existing ring buffer implementations that can be used > from net/base? I don't believe so. Our main socket class (TcpClientSocket) uses a single buffer each for reads and sends, passed in by whatever's using it. It doesn't support multiple sends at once. Instead, it calls a function when a read/send is completed, and it's ready for another request. StreamSocket, which I'm not terribly familiar with, does basically what I suggested - keeps a list of send buffers, and keeps on adding to it. I'm not quite sure what the class is used for.
http://codereview.chromium.org/10389007/diff/5/net/base/tcp_listen_socket.cc File net/base/tcp_listen_socket.cc (right): http://codereview.chromium.org/10389007/diff/5/net/base/tcp_listen_socket.cc#... net/base/tcp_listen_socket.cc:45: 100, If we're sending megabytes locally, 100 milliseconds might be too long.
Added a new TCPListenSocketTest.ServerSendMultiple test to test the buffering and async continuation. http://codereview.chromium.org/10389007/diff/5/net/base/tcp_listen_socket.cc File net/base/tcp_listen_socket.cc (right): http://codereview.chromium.org/10389007/diff/5/net/base/tcp_listen_socket.cc#... net/base/tcp_listen_socket.cc:45: 100, On 2012/05/08 21:23:38, Matt Menke wrote: > If we're sending megabytes locally, 100 milliseconds might be too long. Reduced to 25ms. http://codereview.chromium.org/10389007/diff/5/net/base/tcp_listen_socket.cc#... net/base/tcp_listen_socket.cc:179: } else if (send_data_.size() > kMaxSendBufSize) { On 2012/05/08 20:51:43, Matt Menke wrote: > Should this be send_data_.size() + len? We'd still be allowing the first send > to exceed the limit. Done. http://codereview.chromium.org/10389007/diff/5/net/base/tcp_listen_socket.cc#... net/base/tcp_listen_socket.cc:182: send_data_error_ = true; On 2012/05/08 20:51:43, Matt Menke wrote: > send_data_.clear()? Done.
http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.cc File net/base/tcp_listen_socket.cc (right): http://codereview.chromium.org/10389007/diff/8001/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:222: send_data_ = send_data_.substr(sent, len_left - sent); On 2012/05/08 21:21:35, Matt Menke wrote: > On 2012/05/08 21:04:57, Marshall wrote: > > On 2012/05/08 20:51:43, Matt Menke wrote: > > > On 2012/05/08 20:27:07, Marshall wrote: > > > > On 2012/05/08 17:08:46, Matt Menke wrote: > > > > > This is so ugly...you may want to use a scoped_array of > DrainableIOBuffers > > > or > > > > > something. > > > > > > > > The buffer can grow to > 1MB in size. Do we want to keep a dedicated > > multi-MB > > > > buffer for TCPListenSocket? If so, a circular buffer implementation may be > > the > > > > best option. What do you think? > > > > > > I think that's reasonable. The one problem with a ringbuffer is resizing > one > > is > > > a little tricky, but really shouldn't be too bad at all. Probably a better > > > solution than my suggestion, was just trying to keep things simple. > > > > Are there any currently existing ring buffer implementations that can be used > > from net/base? > > I don't believe so. Our main socket class (TcpClientSocket) uses a single > buffer each for reads and sends, passed in by whatever's using it. It doesn't > support multiple sends at once. Instead, it calls a function when a read/send > is completed, and it's ready for another request. > > StreamSocket, which I'm not terribly familiar with, does basically what I > suggested - keeps a list of send buffers, and keeps on adding to it. I'm not > quite sure what the class is used for. Keeping a list of send buffers sounds like a good idea. We can then limit the total pending data size while also avoiding the allocation/resize of a single large buffer.
On 2012/05/08 21:53:44, Marshall wrote: > Keeping a list of send buffers sounds like a good idea. We can then limit the > total pending data size while also avoiding the allocation/resize of a single > large buffer. Works for me
Changed to use a vector of DrainableIOBuffer instead of std::string for storing pending data.
I'm reasonably happy with this CL, though still a bit concerned about how the backoff timer will affect performance, though I don't have a better suggestion, unfortunately, other than maybe throttling a little more slowly than 2x. Overhead of a couple extra unsuccessful sends shouldn't be too bad. http://codereview.chromium.org/10389007/diff/6005/net/base/tcp_listen_socket.cc File net/base/tcp_listen_socket.cc (right): http://codereview.chromium.org/10389007/diff/6005/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:37: const int kMaxSendBufSize = 1024*1024*5; // 5MB nit: 1024 * 1024 * 5 http://codereview.chromium.org/10389007/diff/6005/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:362: do { I think this may be a little simpler if you used a "while(!send_buffers_.empty())", and removed the if statement above. Alternatively, you could use a while(true), and do the exit check where the continue is now. http://codereview.chromium.org/10389007/diff/6005/net/base/tcp_listen_socket.h File net/base/tcp_listen_socket.h (right): http://codereview.chromium.org/10389007/diff/6005/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.h:114: std::vector<scoped_refptr<DrainableIOBuffer> > send_buffers_; Out of paranoia, might be best to make this a list. http://codereview.chromium.org/10389007/diff/6005/net/base/tcp_listen_socket_... File net/base/tcp_listen_socket_unittest.cc (right): http://codereview.chromium.org/10389007/diff/6005/net/base/tcp_listen_socket_... net/base/tcp_listen_socket_unittest.cc:202: int r = HANDLE_EINTR(recv(test_socket_, buf, buf_len-1, 0)); Looks to me like we never enter the message loop here, so if the tests passes, that means that all the data has already been passed to send, so we may never use the timer, right?
this is looking pretty good to me https://chromiumcodereview.appspot.com/10389007/diff/6005/net/base/tcp_listen... File net/base/tcp_listen_socket.cc (right): https://chromiumcodereview.appspot.com/10389007/diff/6005/net/base/tcp_listen... net/base/tcp_listen_socket.cc:192: if (!send_backoff_.ShouldRejectRequest()) For clarity, consider replacing this test with !send_timer_.IsRunning()... i think thats the more proximate and obvious condition that matters here. If it's possible for ShouldReject to return true when the timer is not running... that would be a problem since the data added above would never get sent. Its not real obvious that ShouldReject (that backoff class is a bit of a black box) won't dont that under some circumstances. https://chromiumcodereview.appspot.com/10389007/diff/6005/net/base/tcp_listen... net/base/tcp_listen_socket.cc:375: if (sent == kSocketError) { Is it possible for send() to return any other error code? If it is this could infinite loop. Maybe we don't need to defend against that in production code (not sure), probably worth at least a DCHECK that sent > 0 || sent == kSocketError. https://chromiumcodereview.appspot.com/10389007/diff/6005/net/base/tcp_listen... net/base/tcp_listen_socket.cc:392: } else if (sent > 0) { nit: might be nice to have the sent everything and sent something cases adjacent to one another, as coded the error handling case is wedged between those two success cases https://chromiumcodereview.appspot.com/10389007/diff/6005/net/base/tcp_listen... net/base/tcp_listen_socket.cc:400: // Data has been sent. nit: some of these comments are stating the obvious, consider removing those that don't really add value https://chromiumcodereview.appspot.com/10389007/diff/6005/net/base/tcp_listen... File net/base/tcp_listen_socket_unittest.cc (right): https://chromiumcodereview.appspot.com/10389007/diff/6005/net/base/tcp_listen... net/base/tcp_listen_socket_unittest.cc:202: int r = HANDLE_EINTR(recv(test_socket_, buf, buf_len-1, 0)); On 2012/05/09 19:20:08, Matt Menke wrote: > Looks to me like we never enter the message loop here... A separate thread is running the listener-side of this test and making the send calls to TCPListenSocket after this thread has called connect(), see the SetUp() method. Given the sycnhronized calls to NextAction(), by the time recv is called here SendFromTester has been invoked |send_count| times and the timer should be running. Might make sense to EXPECT_TRUE that condition prior to entering this loop?
https://chromiumcodereview.appspot.com/10389007/diff/6005/net/base/tcp_listen... File net/base/tcp_listen_socket_unittest.cc (right): https://chromiumcodereview.appspot.com/10389007/diff/6005/net/base/tcp_listen... net/base/tcp_listen_socket_unittest.cc:202: int r = HANDLE_EINTR(recv(test_socket_, buf, buf_len-1, 0)); On 2012/05/09 20:55:34, michaeln wrote: > On 2012/05/09 19:20:08, Matt Menke wrote: > > Looks to me like we never enter the message loop here... > > A separate thread is running the listener-side of this test and making the send > calls to TCPListenSocket after this thread has called connect(), see the SetUp() > method. Given the sycnhronized calls to NextAction(), by the time recv is called > here SendFromTester has been invoked |send_count| times and the timer should be > running. > > Might make sense to EXPECT_TRUE that condition prior to entering this loop? Ahh, thanks! I didn't even notice. So used to seeing only BrowserThread::PostTask and MessageLoop::current()->PostTask, that I just saw that lack of a thread ID and assumed it was the latter. Tests make much more sense now.
http://codereview.chromium.org/10389007/diff/6005/net/base/tcp_listen_socket.cc File net/base/tcp_listen_socket.cc (right): http://codereview.chromium.org/10389007/diff/6005/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:37: const int kMaxSendBufSize = 1024*1024*5; // 5MB On 2012/05/09 19:20:08, Matt Menke wrote: > nit: 1024 * 1024 * 5 Done. http://codereview.chromium.org/10389007/diff/6005/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:192: if (!send_backoff_.ShouldRejectRequest()) On 2012/05/09 20:55:34, michaeln wrote: > For clarity, consider replacing this test with !send_timer_.IsRunning()... i > think thats the more proximate and obvious condition that matters here. > > If it's possible for ShouldReject to return true when the timer is not > running... that would be a problem since the data added above would never get > sent. Its not real obvious that ShouldReject (that backoff class is a bit of a > black box) won't dont that under some circumstances. Done. http://codereview.chromium.org/10389007/diff/6005/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:362: do { On 2012/05/09 19:20:08, Matt Menke wrote: > I think this may be a little simpler if you used a > "while(!send_buffers_.empty())", and removed the if statement above. > > Alternatively, you could use a while(true), and do the exit check where the > continue is now. Done. http://codereview.chromium.org/10389007/diff/6005/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:375: if (sent == kSocketError) { On 2012/05/09 20:55:34, michaeln wrote: > Is it possible for send() to return any other error code? If it is this could > infinite loop. Maybe we don't need to defend against that in production code > (not sure), probably worth at least a DCHECK that sent > 0 || sent == > kSocketError. Done. http://codereview.chromium.org/10389007/diff/6005/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:392: } else if (sent > 0) { On 2012/05/09 20:55:34, michaeln wrote: > nit: might be nice to have the sent everything and sent something cases adjacent > to one another, as coded the error handling case is wedged between those two > success cases Done. http://codereview.chromium.org/10389007/diff/6005/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:400: // Data has been sent. On 2012/05/09 20:55:34, michaeln wrote: > nit: some of these comments are stating the obvious, consider removing those > that don't really add value I've removed some. Let me know if you think more should be removed. http://codereview.chromium.org/10389007/diff/6005/net/base/tcp_listen_socket.h File net/base/tcp_listen_socket.h (right): http://codereview.chromium.org/10389007/diff/6005/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.h:114: std::vector<scoped_refptr<DrainableIOBuffer> > send_buffers_; On 2012/05/09 19:20:08, Matt Menke wrote: > Out of paranoia, might be best to make this a list. Done.
lgtm, i just have some style nits, please do with them as you see fit and thnx for slogging thru this http://codereview.chromium.org/10389007/diff/9/net/base/tcp_listen_socket.cc File net/base/tcp_listen_socket.cc (right): http://codereview.chromium.org/10389007/diff/9/net/base/tcp_listen_socket.cc#... net/base/tcp_listen_socket.cc:178: // Too much of a backup, stop trying to add more data. nit: given the log error message immediately below and the easy to read condition in the if statement, maybe remove this comment http://codereview.chromium.org/10389007/diff/9/net/base/tcp_listen_socket.cc#... net/base/tcp_listen_socket.cc:354: // This method should never be called when the send buffer is empty. nit: the dcheck effectively says this http://codereview.chromium.org/10389007/diff/9/net/base/tcp_listen_socket.cc#... net/base/tcp_listen_socket.cc:374: buffer->DidConsume(sent); style nit: would less text be more readable? if (sent == len_left) send_buffers_.erase(...); else buffer->DidConsume(send); http://codereview.chromium.org/10389007/diff/9/net/base/tcp_listen_socket.cc#... net/base/tcp_listen_socket.cc:406: send_backoff_.InformOfRequest(false); the comments about how these affect the back-off delay are useful since the method names aren't obvious (at least to me) http://codereview.chromium.org/10389007/diff/9/net/base/tcp_listen_socket.cc#... net/base/tcp_listen_socket.cc:409: if (!send_buffers_.empty()) { maybe DCHECK_GT(send_pending_size_, 0)
LGTM as well http://codereview.chromium.org/10389007/diff/9/net/base/tcp_listen_socket.cc File net/base/tcp_listen_socket.cc (right): http://codereview.chromium.org/10389007/diff/9/net/base/tcp_listen_socket.cc#... net/base/tcp_listen_socket.cc:371: send_buffers_.erase(send_buffers_.begin()); nit: send_buffers_.pop_front() http://codereview.chromium.org/10389007/diff/9/net/base/tcp_listen_socket_uni... File net/base/tcp_listen_socket_unittest.cc (right): http://codereview.chromium.org/10389007/diff/9/net/base/tcp_listen_socket_uni... net/base/tcp_listen_socket_unittest.cc:201: do { nit: Suggest a while loop here. In my opinion, they're a little easier to follow, and it's more consistent with the function above.
http://codereview.chromium.org/10389007/diff/6005/net/base/tcp_listen_socket.cc File net/base/tcp_listen_socket.cc (right): http://codereview.chromium.org/10389007/diff/6005/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:375: if (sent == kSocketError) { On 2012/05/09 20:55:34, michaeln wrote: > Is it possible for send() to return any other error code? If it is this could > infinite loop. Maybe we don't need to defend against that in production code > (not sure), probably worth at least a DCHECK that sent > 0 || sent == > kSocketError. I changed this to an "else { NOTREACHED(); break; }" so that if it does happen we don't get stuck in an infinite loop. http://codereview.chromium.org/10389007/diff/9/net/base/tcp_listen_socket.cc File net/base/tcp_listen_socket.cc (right): http://codereview.chromium.org/10389007/diff/9/net/base/tcp_listen_socket.cc#... net/base/tcp_listen_socket.cc:178: // Too much of a backup, stop trying to add more data. On 2012/05/10 16:52:21, michaeln wrote: > nit: given the log error message immediately below and the easy to read > condition in the if statement, maybe remove this comment Done. http://codereview.chromium.org/10389007/diff/9/net/base/tcp_listen_socket.cc#... net/base/tcp_listen_socket.cc:354: // This method should never be called when the send buffer is empty. On 2012/05/10 16:52:21, michaeln wrote: > nit: the dcheck effectively says this Done. http://codereview.chromium.org/10389007/diff/9/net/base/tcp_listen_socket.cc#... net/base/tcp_listen_socket.cc:371: send_buffers_.erase(send_buffers_.begin()); On 2012/05/10 17:17:19, Matt Menke wrote: > nit: send_buffers_.pop_front() Done. http://codereview.chromium.org/10389007/diff/9/net/base/tcp_listen_socket.cc#... net/base/tcp_listen_socket.cc:374: buffer->DidConsume(sent); On 2012/05/10 16:52:21, michaeln wrote: > style nit: would less text be more readable? > > if (sent == len_left) > send_buffers_.erase(...); > else > buffer->DidConsume(send); Done. http://codereview.chromium.org/10389007/diff/9/net/base/tcp_listen_socket.cc#... net/base/tcp_listen_socket.cc:409: if (!send_buffers_.empty()) { On 2012/05/10 16:52:21, michaeln wrote: > maybe DCHECK_GT(send_pending_size_, 0) That's probably not necessary because we DCHECK at the time that we change the send_pending_size_ value.
And still LGTM
still lgtm2, thnx marshall and please run some tryjobs prior to committing
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/marshall@chromium.org/10389007/11009
Try job failure for 10389007-11009 (retry) on android for steps "Compile, build". It's a second try, previously, steps "Compile, build" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/marshall@chromium.org/10389007/11009
Change committed as 136572
On 2012/05/11 15:52:48, I haz the power (commit-bot) wrote: > Change committed as 136572 This change appears to have broken UrlmonUrlRequestTest.UnreachableUrl. http://build.chromium.org/p/chromium/builders/Chrome%20Frame%20Tests%20%28ie6...
On 2012/05/11 17:02:00, benjhayden_chromium wrote: > On 2012/05/11 15:52:48, I haz the power (commit-bot) wrote: > > Change committed as 136572 > > This change appears to have broken UrlmonUrlRequestTest.UnreachableUrl. > http://build.chromium.org/p/chromium/builders/Chrome%2520Frame%2520Tests%2520... I agree with your assessment. The code this touches is used as part of the test infrastructure, rather than in production code, so it's not an actual regression. I'm on the fence as to whether we should temporarily disable the test, or revert the CL.
bummer... i guess there is an existing consumer that's unhappy with the change :( |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
