Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8526006:
Close idle sockets next time we are about to send data. (Closed)
|
Created:
9 years, 1 month ago by selim Modified:
9 years, 1 month ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, Paweł Hajdan Jr. Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionClose idle sockets next time we are about to send data.
This change enables closing idle sockets when client initiates a new socket request rather than using a timer based approach. This prevents waking up network interface only for the purpose of sending a FIN to the server.
BUG=101820
TEST=unit-tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110250
Patch Set 1 #
Total comments: 34
Patch Set 2 : addressed code review #
Total comments: 3
Patch Set 3 : addresses one more code convention issue #Patch Set 4 : address code review about stale idle sockets #
Total comments: 3
Patch Set 5 : refactored #Patch Set 6 : code conventions #
Total comments: 6
Patch Set 7 : use global variable. #
Total comments: 6
Patch Set 8 : addressed code review #
Messages
Total messages: 29 (0 generated)
I appreciate it if you can review my first ever chromium patch at your first convenient time. Please let me know how to improve further. Best, -Selim
Matt, I'm going to let you take a first pass at this. Thanks for all the code reviewing help, you're doing a great job! Selim, I've skimmed your tryjob results. I think the broken ones on win/linux are flake. So don't worry about them. On 2011/11/10 19:12:53, selim wrote: > I appreciate it if you can review my first ever chromium patch at your first > convenient time. Please let me know how to improve further. > > Best, > > -Selim
Overall looks good, just a bunch of style nits. I have yet to more than glance at the unit test. http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.cc:196: use_cleanup_timer_ = false; Just for consistency, and so it's only modified in one place, I'd suggest using set_cleanup_timer_enabled(false) here instead. http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.cc:256: if (!use_cleanup_timer_) CleanupIdleSockets(false); nit: Google style is to always split ifs onto two lines. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Conditionals http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.cc:305: if (!use_cleanup_timer_) CleanupIdleSockets(false); nit: Google style is to always split ifs onto two lines. http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.cc:718: return use_cleanup_timer_; nit: non-virtual one-liners like this are generally inlined in the header files. http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.cc:721: void ClientSocketPoolBaseHelper::set_cleanup_timer_enabled( bool enabled ) { nit: "(bool enabled)" http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.cc:721: void ClientSocketPoolBaseHelper::set_cleanup_timer_enabled( bool enabled ) { nit: Try to put these in the same order relative to other functions as they are in the header files. This often isn't possible to do perfectly, as a result of refactorings, but can at least put it before or after one of the same functions. http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.cc:727: timer_.Stop(); nit: If you use braces on the first block of a conditional statement, always use them on the second block, too. http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... File net/socket/client_socket_pool_base.h (right): http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.h:320: // idle sockets that has been around for longer than a period nit: Maybe 'idle sockets that have been around too long.' (has -> have, and it's a little weird to mention use 'a period' without more elaboration on just how long it is). http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.h:324: bool cleanup_timer_enabled(); Put these functions in the same location relative to the other functions as you use below, or vice versa. http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.h:324: bool cleanup_timer_enabled(); nit: Use const whenever appropriate. http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.h:325: void set_cleanup_timer_enabled( bool enabled ); Since this is more than a simple setter, use SetCleanupTimerEnabled(), and for the functions that wrap it as well. http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.h:325: void set_cleanup_timer_enabled( bool enabled ); nit: Google style is "(bool enabled)". See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...) http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.h:454: void startIdleSocketTimer(); nit: Google style is "StartIdleSocketTimer()". See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.h:722: bool cleanup_timer_enabled() { nit: const http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.h:726: void set_cleanup_timer_enabled( bool enabled ) { nit: Google style is "(bool enabled)" http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.h:727: return helper_.set_cleanup_timer_enabled( enabled ); nit: Google style is "(enabled)" http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... File net/socket/client_socket_pool_base_unittest.cc (right): http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base_unittest.cc:1956: base::TimeDelta::FromMilliseconds(10), // Time out unused sockets nit: Google style is two spaces between code and comments. http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base_unittest.cc:1959: // disable cleanup timer nit: "Disable cleanup timer."
This may seem like a lot of nits, but everyone starts out getting a lot before they acclimate to the Google style.
The unit test looks fine to me.
Hi Matt, Thanks for your very detailed review. I believe I addressed all the comments and ready for a second round. Thanks, http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.cc:196: use_cleanup_timer_ = false; On 2011/11/11 03:33:01, Matt Menke wrote: > Just for consistency, and so it's only modified in one place, I'd suggest using > set_cleanup_timer_enabled(false) here instead. done http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.cc:256: if (!use_cleanup_timer_) CleanupIdleSockets(false); On 2011/11/11 03:33:01, Matt Menke wrote: > nit: Google style is to always split ifs onto two lines. > > See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Conditionals done. Even though I find it less readable, code conventions is code conventions and there is no way around! http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.cc:305: if (!use_cleanup_timer_) CleanupIdleSockets(false); On 2011/11/11 03:33:01, Matt Menke wrote: > nit: Google style is to always split ifs onto two lines. done http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.cc:718: return use_cleanup_timer_; On 2011/11/11 03:33:01, Matt Menke wrote: > nit: non-virtual one-liners like this are generally inlined in the header > files. done http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.cc:721: void ClientSocketPoolBaseHelper::set_cleanup_timer_enabled( bool enabled ) { On 2011/11/11 03:33:01, Matt Menke wrote: > nit: "(bool enabled)" done http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.cc:727: timer_.Stop(); On 2011/11/11 03:33:01, Matt Menke wrote: > nit: If you use braces on the first block of a conditional statement, always > use them on the second block, too. done http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... File net/socket/client_socket_pool_base.h (right): http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.h:320: // idle sockets that has been around for longer than a period On 2011/11/11 03:33:01, Matt Menke wrote: > nit: Maybe 'idle sockets that have been around too long.' > (has -> have, and it's a little weird to mention use 'a period' without more > elaboration on just how long it is). done http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.h:324: bool cleanup_timer_enabled(); On 2011/11/11 03:33:01, Matt Menke wrote: > nit: Use const whenever appropriate. done http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.h:324: bool cleanup_timer_enabled(); On 2011/11/11 03:33:01, Matt Menke wrote: > Put these functions in the same location relative to the other functions as you > use below, or vice versa. done http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.h:325: void set_cleanup_timer_enabled( bool enabled ); On 2011/11/11 03:33:01, Matt Menke wrote: > Since this is more than a simple setter, use SetCleanupTimerEnabled(), and for > the functions that wrap it as well. done http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.h:454: void startIdleSocketTimer(); On 2011/11/11 03:33:01, Matt Menke wrote: > nit: Google style is "StartIdleSocketTimer()". See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names done http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.h:722: bool cleanup_timer_enabled() { On 2011/11/11 03:33:01, Matt Menke wrote: > nit: const done http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.h:726: void set_cleanup_timer_enabled( bool enabled ) { On 2011/11/11 03:33:01, Matt Menke wrote: > nit: Google style is "(bool enabled)" done http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base.h:727: return helper_.set_cleanup_timer_enabled( enabled ); On 2011/11/11 03:33:01, Matt Menke wrote: > nit: Google style is "(enabled)" done http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... File net/socket/client_socket_pool_base_unittest.cc (right): http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base_unittest.cc:1956: base::TimeDelta::FromMilliseconds(10), // Time out unused sockets On 2011/11/11 03:33:01, Matt Menke wrote: > nit: Google style is two spaces between code and comments. done http://codereview.chromium.org/8526006/diff/1/net/socket/client_socket_pool_b... net/socket/client_socket_pool_base_unittest.cc:1959: // disable cleanup timer On 2011/11/11 03:33:01, Matt Menke wrote: > nit: "Disable cleanup timer." done
LGTM, module one nit. http://codereview.chromium.org/8526006/diff/4003/net/socket/client_socket_poo... File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/8526006/diff/4003/net/socket/client_socket_poo... net/socket/client_socket_pool_base.cc:257: CleanupIdleSockets(false); Wonder if potentially using a more stale than normal socket here could have a performance impact. May be interesting to see if calling this at the start of this function instead makes any difference, at some point in the future. http://codereview.chromium.org/8526006/diff/4003/net/socket/client_socket_poo... File net/socket/client_socket_pool_base.h (right): http://codereview.chromium.org/8526006/diff/4003/net/socket/client_socket_poo... net/socket/client_socket_pool_base.h:302: void SetCleanupTimerEnabled( bool enabled ); nit: "(bool enabled)"
addressed. please see my comment about performance, if there is anything we can do to make it better now, let's do it. http://codereview.chromium.org/8526006/diff/4003/net/socket/client_socket_poo... File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/8526006/diff/4003/net/socket/client_socket_poo... net/socket/client_socket_pool_base.cc:257: CleanupIdleSockets(false); On 2011/11/11 20:19:20, Matt Menke wrote: > Wonder if potentially using a more stale than normal socket here could have a > performance impact. May be interesting to see if calling this at the start of > this function instead makes any difference, at some point in the future. I think, if we call cleanup idle sockets first, then under some circumstances, it can cleanup all idle sockets and the group itself. Then we have to re-create the group and create a socket immediately, which is slightly less efficient in terms of CPU usage. I guess your comment is more about network performance. Why would an idle socket perform worse?
On 2011/11/11 21:33:05, selim wrote: > addressed. please see my comment about performance, if there is anything we can > do to make it better now, let's do it. > > http://codereview.chromium.org/8526006/diff/4003/net/socket/client_socket_poo... > File net/socket/client_socket_pool_base.cc (right): > > http://codereview.chromium.org/8526006/diff/4003/net/socket/client_socket_poo... > net/socket/client_socket_pool_base.cc:257: CleanupIdleSockets(false); > On 2011/11/11 20:19:20, Matt Menke wrote: > > Wonder if potentially using a more stale than normal socket here could have a > > performance impact. May be interesting to see if calling this at the start of > > this function instead makes any difference, at some point in the future. > > I think, if we call cleanup idle sockets first, then under some circumstances, > it can cleanup all idle sockets and the group itself. Then we have to re-create > the group and create a socket immediately, which is slightly less efficient in > terms of CPU usage. I guess your comment is more about network performance. Why > would an idle socket perform worse? It could have timed out, and we just weren't aware of it, for whatever reason, in which case we'd have to wait to get an RST packet back from the server before reconnecting. I'm no TCP/IP expert, and I'm not sure how likely this is to happen on a desktop, let alone mobile, but I could see it being a potential issue with very stale sockets.
I think it can happen if the peer somehow closed the connection without being able to send a FIN. I don't know if it can happen otherwise. In this case it adds one RTT for the RST. So let's simply change it rather than leaving it to later. On 2011/11/11 21:40:18, Matt Menke wrote: > On 2011/11/11 21:33:05, selim wrote: > > addressed. please see my comment about performance, if there is anything we > can > > do to make it better now, let's do it. > > > > > http://codereview.chromium.org/8526006/diff/4003/net/socket/client_socket_poo... > > File net/socket/client_socket_pool_base.cc (right): > > > > > http://codereview.chromium.org/8526006/diff/4003/net/socket/client_socket_poo... > > net/socket/client_socket_pool_base.cc:257: CleanupIdleSockets(false); > > On 2011/11/11 20:19:20, Matt Menke wrote: > > > Wonder if potentially using a more stale than normal socket here could have > a > > > performance impact. May be interesting to see if calling this at the start > of > > > this function instead makes any difference, at some point in the future. > > > > I think, if we call cleanup idle sockets first, then under some circumstances, > > it can cleanup all idle sockets and the group itself. Then we have to > re-create > > the group and create a socket immediately, which is slightly less efficient in > > terms of CPU usage. I guess your comment is more about network performance. > Why > > would an idle socket perform worse? > > It could have timed out, and we just weren't aware of it, for whatever reason, > in which case we'd have to wait to get an RST packet back from the server before > reconnecting. I'm no TCP/IP expert, and I'm not sure how likely this is to > happen on a desktop, let alone mobile, but I could see it being a potential > issue with very stale sockets.
addressed code review about stale sockets.
On 2011/11/11 22:12:49, selim wrote: > I think it can happen if the peer somehow closed the connection without being > able to send a FIN. I don't know if it can happen otherwise. In this case it > adds one RTT for the RST. > So let's simply change it rather than leaving it to later. I'm not actually sure if it's a good idea to do it first or not, which is why I suggested gathering some data one it. First pass, I'm happy either way.
On 2011/11/11 21:40:18, Matt Menke wrote: > On 2011/11/11 21:33:05, selim wrote: > > addressed. please see my comment about performance, if there is anything we > can > > do to make it better now, let's do it. > > > > > http://codereview.chromium.org/8526006/diff/4003/net/socket/client_socket_poo... > > File net/socket/client_socket_pool_base.cc (right): > > > > > http://codereview.chromium.org/8526006/diff/4003/net/socket/client_socket_poo... > > net/socket/client_socket_pool_base.cc:257: CleanupIdleSockets(false); > > On 2011/11/11 20:19:20, Matt Menke wrote: > > > Wonder if potentially using a more stale than normal socket here could have > a > > > performance impact. May be interesting to see if calling this at the start > of > > > this function instead makes any difference, at some point in the future. > > > > I think, if we call cleanup idle sockets first, then under some circumstances, > > it can cleanup all idle sockets and the group itself. Then we have to > re-create > > the group and create a socket immediately, which is slightly less efficient in > > terms of CPU usage. I guess your comment is more about network performance. > Why > > would an idle socket perform worse? > > It could have timed out, and we just weren't aware of it, for whatever reason, > in which case we'd have to wait to get an RST packet back from the server before > reconnecting. I'm no TCP/IP expert, and I'm not sure how likely this is to > happen on a desktop, let alone mobile, but I could see it being a potential > issue with very stale sockets. Yes, this may happen. We've run experiments before on how long we should keep an idle socket alive in order to maximize performance. The current values reflect A/B experiments run a little over a year ago.
http://codereview.chromium.org/8526006/diff/8002/net/socket/client_socket_poo... File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/8526006/diff/8002/net/socket/client_socket_poo... net/socket/client_socket_pool_base.cc:196: SetCleanupTimerEnabled(false); I think you should do this differently. The problem with this is there's no way to change this for OS_ANDROID then. There's no public API (ClientSocketPoolBaseHelper is an internal class, not exposed to net consumers) to tweak this. Either we should be passing this in as a parameter to the constructor, or we should be initializing the member variable from a global (this is a pattern we've done elsewhere) in the constructor. I think the "right" thing to do is pass it in as a constructor argument. That might require a bunch of changes to existing ClientSocketPool classes. I'm find deferring that change due to time constraints, but would like it to be done eventually. The expedient change would be to just use a global variable (see g_connect_backup_jobs_enabled). http://codereview.chromium.org/8526006/diff/8002/net/socket/client_socket_poo... net/socket/client_socket_pool_base.cc:246: CleanupIdleSockets(false); Move this after the BeginEvent(). http://codereview.chromium.org/8526006/diff/8002/net/socket/client_socket_poo... net/socket/client_socket_pool_base.cc:720: void ClientSocketPoolBaseHelper::SetCleanupTimerEnabled(bool enabled) { I think it's not necessary to have this code where you can dynamically turn on/off cleanup timers. I think it's OK for us to just assume it never changes after construction, and furthermore, I'd rather you do that because it makes this already complicated code easier to reason about.
On 2011/11/11 23:47:50, willchan wrote: > http://codereview.chromium.org/8526006/diff/8002/net/socket/client_socket_poo... > File net/socket/client_socket_pool_base.cc (right): > > http://codereview.chromium.org/8526006/diff/8002/net/socket/client_socket_poo... > net/socket/client_socket_pool_base.cc:196: SetCleanupTimerEnabled(false); > I think you should do this differently. The problem with this is there's no way > to change this for OS_ANDROID then. There's no public API > (ClientSocketPoolBaseHelper is an internal class, not exposed to net consumers) > to tweak this. Either we should be passing this in as a parameter to the > constructor, or we should be initializing the member variable from a global > (this is a pattern we've done elsewhere) in the constructor. > > I think the "right" thing to do is pass it in as a constructor argument. That > might require a bunch of changes to existing ClientSocketPool classes. I'm find > deferring that change due to time constraints, but would like it to be done > eventually. The expedient change would be to just use a global variable (see > g_connect_backup_jobs_enabled). > > http://codereview.chromium.org/8526006/diff/8002/net/socket/client_socket_poo... > net/socket/client_socket_pool_base.cc:246: CleanupIdleSockets(false); > Move this after the BeginEvent(). > > http://codereview.chromium.org/8526006/diff/8002/net/socket/client_socket_poo... > net/socket/client_socket_pool_base.cc:720: void > ClientSocketPoolBaseHelper::SetCleanupTimerEnabled(bool enabled) { > I think it's not necessary to have this code where you can dynamically turn > on/off cleanup timers. I think it's OK for us to just assume it never changes > after construction, and furthermore, I'd rather you do that because it makes > this already complicated code easier to reason about. I actually went back and forth between global variable, constructor or having public methods to enable/disable timers. Note that the public methods actually provide a way to Android (or any other OS) to disable or enable the cleanup timer. But if we are worried about code complication, yes, I agree that we should change it.
On Fri, Nov 11, 2011 at 4:06 PM, <sgurun@google.com> wrote: > On 2011/11/11 23:47:50, willchan wrote: > > http://codereview.chromium.**org/8526006/diff/8002/net/** > socket/client_socket_pool_**base.cc<http://codereview.chromium.org/8526006/diff/8002/net/socket/client_socket_pool_base.cc> > >> File net/socket/client_socket_pool_**base.cc (right): >> > > > http://codereview.chromium.**org/8526006/diff/8002/net/** > socket/client_socket_pool_**base.cc#newcode196<http://codereview.chromium.org/8526006/diff/8002/net/socket/client_socket_pool_base.cc#newcode196> > >> net/socket/client_socket_pool_**base.cc:196: >> SetCleanupTimerEnabled(false); >> I think you should do this differently. The problem with this is there's >> no >> > way > >> to change this for OS_ANDROID then. There's no public API >> (ClientSocketPoolBaseHelper is an internal class, not exposed to net >> > consumers) > >> to tweak this. Either we should be passing this in as a parameter to the >> constructor, or we should be initializing the member variable from a >> global >> (this is a pattern we've done elsewhere) in the constructor. >> > > I think the "right" thing to do is pass it in as a constructor argument. >> That >> might require a bunch of changes to existing ClientSocketPool classes. I'm >> > find > >> deferring that change due to time constraints, but would like it to be >> done >> eventually. The expedient change would be to just use a global variable >> (see >> g_connect_backup_jobs_enabled)**. >> > > > http://codereview.chromium.**org/8526006/diff/8002/net/** > socket/client_socket_pool_**base.cc#newcode246<http://codereview.chromium.org/8526006/diff/8002/net/socket/client_socket_pool_base.cc#newcode246> > >> net/socket/client_socket_pool_**base.cc:246: CleanupIdleSockets(false); >> Move this after the BeginEvent(). >> > > > http://codereview.chromium.**org/8526006/diff/8002/net/** > socket/client_socket_pool_**base.cc#newcode720<http://codereview.chromium.org/8526006/diff/8002/net/socket/client_socket_pool_base.cc#newcode720> > >> net/socket/client_socket_pool_**base.cc:720: void >> ClientSocketPoolBaseHelper::**SetCleanupTimerEnabled(bool enabled) { >> I think it's not necessary to have this code where you can dynamically >> turn >> on/off cleanup timers. I think it's OK for us to just assume it never >> changes >> after construction, and furthermore, I'd rather you do that because it >> makes >> this already complicated code easier to reason about. >> > > I actually went back and forth between global variable, constructor or > having > public methods to enable/disable timers. Note that the public methods > actually > provide a way to Android (or any other OS) to disable or enable the cleanup > timer. But if we are worried about code complication, yes, I agree that we > should change it. > Yes, I'd like to simplify the code. Also, the public methods are only exposed in an internal class (ClientSocketPoolBaseHelper, marked with NET_EXPORT_PRIVATE), not in a class with NET_EXPORT. No code outside of net/ is allowed to access a class marked by NET_EXPORT_PRIVATE. > > http://codereview.chromium.**org/8526006/<http://codereview.chromium.org/8526... >
Hi, I did some refactoring to remove ifdef ANDROID_OS and pass cleanup mode as part of an option struct through constructor. This makes it possible to configure HttpNetworkSession during initialization. I appreciate it if you can take a look at it and give me feedback. Thanks,
Lots of style nits as per style guide. http://codereview.chromium.org/8526006/diff/14001/net/http/http_cache.h File net/http/http_cache.h (right): http://codereview.chromium.org/8526006/diff/14001/net/http/http_cache.h#newco... net/http/http_cache.h:143: IdleSocketCleanupMode cleanup_mode = USE_CLEANUP_TIMER); default args are not allowed by the style guide http://codereview.chromium.org/8526006/diff/14001/net/http/http_network_sessi... File net/http/http_network_session.h (right): http://codereview.chromium.org/8526006/diff/14001/net/http/http_network_sessi... net/http/http_network_session.h:80: This newline is spurious http://codereview.chromium.org/8526006/diff/14001/net/http/http_proxy_client_... File net/http/http_proxy_client_socket_pool.h (right): http://codereview.chromium.org/8526006/diff/14001/net/http/http_proxy_client_... net/http/http_proxy_client_socket_pool.h:180: const ClientSocketPoolOptions& options = ClientSocketPoolOptions()); no default args http://codereview.chromium.org/8526006/diff/14001/net/socket/client_socket_po... File net/socket/client_socket_pool_options.h (right): http://codereview.chromium.org/8526006/diff/14001/net/socket/client_socket_po... net/socket/client_socket_pool_options.h:13: ClientSocketPoolOptions(bool timer_enabled = true) no default args http://codereview.chromium.org/8526006/diff/14001/net/socket/client_socket_po... net/socket/client_socket_pool_options.h:14: : use_cleanup_timer(timer_enabled) {} indent 4 spaces http://codereview.chromium.org/8526006/diff/14001/net/socket/client_socket_po... net/socket/client_socket_pool_options.h:15: virtual ~ClientSocketPoolOptions() {} No need for virtual
Thanks for very quick response. I have to admit that I did not know that default args are not allowed. This significantly increases the scope of the work, because of the way unit tests are written. (In fact I tried not using default args and gave up after seeing the number of issues). One alternative is possibly using another constructor that does not take option, hopefully this is allowed. If none of these works, then we may need to go back to initial global variable solution maybe. On 2011/11/15 20:35:29, willchan wrote: > Lots of style nits as per style guide. > > http://codereview.chromium.org/8526006/diff/14001/net/http/http_cache.h > File net/http/http_cache.h (right): > > http://codereview.chromium.org/8526006/diff/14001/net/http/http_cache.h#newco... > net/http/http_cache.h:143: IdleSocketCleanupMode cleanup_mode = > USE_CLEANUP_TIMER); > default args are not allowed by the style guide > > http://codereview.chromium.org/8526006/diff/14001/net/http/http_network_sessi... > File net/http/http_network_session.h (right): > > http://codereview.chromium.org/8526006/diff/14001/net/http/http_network_sessi... > net/http/http_network_session.h:80: > This newline is spurious > > http://codereview.chromium.org/8526006/diff/14001/net/http/http_proxy_client_... > File net/http/http_proxy_client_socket_pool.h (right): > > http://codereview.chromium.org/8526006/diff/14001/net/http/http_proxy_client_... > net/http/http_proxy_client_socket_pool.h:180: const ClientSocketPoolOptions& > options = ClientSocketPoolOptions()); > no default args > > http://codereview.chromium.org/8526006/diff/14001/net/socket/client_socket_po... > File net/socket/client_socket_pool_options.h (right): > > http://codereview.chromium.org/8526006/diff/14001/net/socket/client_socket_po... > net/socket/client_socket_pool_options.h:13: ClientSocketPoolOptions(bool > timer_enabled = true) > no default args > > http://codereview.chromium.org/8526006/diff/14001/net/socket/client_socket_po... > net/socket/client_socket_pool_options.h:14: : use_cleanup_timer(timer_enabled) > {} > indent 4 spaces > > http://codereview.chromium.org/8526006/diff/14001/net/socket/client_socket_po... > net/socket/client_socket_pool_options.h:15: virtual ~ClientSocketPoolOptions() > {} > No need for virtual
Overloading functions (including constructors) are basically equivalent to default arg values, and so are disallowed by the style guide too. I think that the global variable solution, while suboptimal, is the best option for now. On Tue, Nov 15, 2011 at 3:42 PM, <sgurun@google.com> wrote: > Thanks for very quick response. > > I have to admit that I did not know that default args are not allowed. This > significantly increases the scope of the work, because of the way unit > tests are > written. (In fact I tried not using default args and gave up after seeing > the > number of issues). > > One alternative is possibly using another constructor that does not take > option, > hopefully this is allowed. > > If none of these works, then we may need to go back to initial global > variable > solution maybe. > > > > On 2011/11/15 20:35:29, willchan wrote: > >> Lots of style nits as per style guide. >> > > http://codereview.chromium.**org/8526006/diff/14001/net/** >> http/http_cache.h<http://codereview.chromium.org/8526006/diff/14001/net/http/http_cache.h> >> File net/http/http_cache.h (right): >> > > > http://codereview.chromium.**org/8526006/diff/14001/net/** > http/http_cache.h#newcode143<http://codereview.chromium.org/8526006/diff/14001/net/http/http_cache.h#newcode143> > >> net/http/http_cache.h:143: IdleSocketCleanupMode cleanup_mode = >> USE_CLEANUP_TIMER); >> default args are not allowed by the style guide >> > > > http://codereview.chromium.**org/8526006/diff/14001/net/** > http/http_network_session.h<http://codereview.chromium.org/8526006/diff/14001/net/http/http_network_session.h> > >> File net/http/http_network_session.**h (right): >> > > > http://codereview.chromium.**org/8526006/diff/14001/net/** > http/http_network_session.h#**newcode80<http://codereview.chromium.org/8526006/diff/14001/net/http/http_network_session.h#newcode80> > >> net/http/http_network_session.**h:80: >> This newline is spurious >> > > > http://codereview.chromium.**org/8526006/diff/14001/net/** > http/http_proxy_client_socket_**pool.h<http://codereview.chromium.org/8526006/diff/14001/net/http/http_proxy_client_socket_pool.h> > >> File net/http/http_proxy_client_**socket_pool.h (right): >> > > > http://codereview.chromium.**org/8526006/diff/14001/net/** > http/http_proxy_client_socket_**pool.h#newcode180<http://codereview.chromium.org/8526006/diff/14001/net/http/http_proxy_client_socket_pool.h#newcode180> > >> net/http/http_proxy_client_**socket_pool.h:180: const >> ClientSocketPoolOptions& >> options = ClientSocketPoolOptions()); >> no default args >> > > > http://codereview.chromium.**org/8526006/diff/14001/net/** > socket/client_socket_pool_**options.h<http://codereview.chromium.org/8526006/diff/14001/net/socket/client_socket_pool_options.h> > >> File net/socket/client_socket_pool_**options.h (right): >> > > > http://codereview.chromium.**org/8526006/diff/14001/net/** > socket/client_socket_pool_**options.h#newcode13<http://codereview.chromium.org/8526006/diff/14001/net/socket/client_socket_pool_options.h#newcode13> > >> net/socket/client_socket_pool_**options.h:13: >> ClientSocketPoolOptions(bool >> timer_enabled = true) >> no default args >> > > > http://codereview.chromium.**org/8526006/diff/14001/net/** > socket/client_socket_pool_**options.h#newcode14<http://codereview.chromium.org/8526006/diff/14001/net/socket/client_socket_pool_options.h#newcode14> > >> net/socket/client_socket_pool_**options.h:14: : use_cleanup_timer(timer_* >> *enabled) >> {} >> indent 4 spaces >> > > > http://codereview.chromium.**org/8526006/diff/14001/net/** > socket/client_socket_pool_**options.h#newcode15<http://codereview.chromium.org/8526006/diff/14001/net/socket/client_socket_pool_options.h#newcode15> > >> net/socket/client_socket_pool_**options.h:15: virtual >> ~ClientSocketPoolOptions() >> {} >> No need for virtual >> > > > > http://codereview.chromium.**org/8526006/<http://codereview.chromium.org/8526... >
I'd recommend you carefully read through the style guide. While it doesn't cover everything, it does cover a lot, and it includes explanations for the various rules, which can be pretty helpful, too. On 2011/11/15 20:42:28, selim wrote: > Thanks for very quick response. > > I have to admit that I did not know that default args are not allowed. This > significantly increases the scope of the work, because of the way unit tests are > written. (In fact I tried not using default args and gave up after seeing the > number of issues). > > One alternative is possibly using another constructor that does not take option, > hopefully this is allowed. > > If none of these works, then we may need to go back to initial global variable > solution maybe.
I will change it to global variable. As a side note, I can see the downsides of using default behavior, but there are trade offs, and here for example, it now forces us to use (in my personal opinion) let's desirable solution. However, it looks the conventions are very strict. Thanks for all the input! On 2011/11/15 20:52:04, Matt Menke wrote: > I'd recommend you carefully read through the style guide. While it > doesn't cover everything, it does cover a lot, and it includes > explanations for the various rules, which can be pretty helpful, too. > > On 2011/11/15 20:42:28, selim wrote: > > Thanks for very quick response. > > > > I have to admit that I did not know that default args are not allowed. This > > significantly increases the scope of the work, because of the way unit tests > are > > written. (In fact I tried not using default args and gave up after seeing the > > number of issues). > > > > One alternative is possibly using another constructor that does not take > option, > > hopefully this is allowed. > > > > If none of these works, then we may need to go back to initial global variable > > solution maybe.
Ok, reverted the changes to use global variable and looked at any possible "nit"s. Can you please take a look at the changes and give feedback? It would be wonderful if we can pull these to Android tomorrow. Thanks for your understanding and patience.
http://codereview.chromium.org/8526006/diff/12020/net/socket/client_socket_po... File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/8526006/diff/12020/net/socket/client_socket_po... net/socket/client_socket_pool_base.cc:28: bool g_cleanup_timer_enabled = true; We should probably set a different default for windows. http://codereview.chromium.org/8526006/diff/12020/net/socket/client_socket_po... File net/socket/client_socket_pool_base_unittest.cc (right): http://codereview.chromium.org/8526006/diff/12020/net/socket/client_socket_po... net/socket/client_socket_pool_base_unittest.cc:2000: // We post all of our delayed tasks with a 2ms delay. I.e. they don't Is this comment true?
Here is my comments: http://codereview.chromium.org/8526006/diff/12020/net/socket/client_socket_po... File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/8526006/diff/12020/net/socket/client_socket_po... net/socket/client_socket_pool_base.cc:28: bool g_cleanup_timer_enabled = true; On 2011/11/16 00:51:40, willchan wrote: > We should probably set a different default for windows. We want the timer enabled (true) for windows due to the windows XP bug. I don't know if other desktop OSes want this change or not. So by default it is left as true. We will explicitly set it to false for Android. http://codereview.chromium.org/8526006/diff/12020/net/socket/client_socket_po... File net/socket/client_socket_pool_base_unittest.cc (right): http://codereview.chromium.org/8526006/diff/12020/net/socket/client_socket_po... net/socket/client_socket_pool_base_unittest.cc:2000: // We post all of our delayed tasks with a 2ms delay. I.e. they don't On 2011/11/16 00:51:40, willchan wrote: > Is this comment true? I took this part of the test exactly from the one immediate below. I don't know if this comment is still valid or not.
LGTM, fix up the rest and go ahead and send it to the commit queue. http://codereview.chromium.org/8526006/diff/12020/net/socket/client_socket_po... File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/8526006/diff/12020/net/socket/client_socket_po... net/socket/client_socket_pool_base.cc:28: bool g_cleanup_timer_enabled = true; On 2011/11/16 01:01:08, selim wrote: > On 2011/11/16 00:51:40, willchan wrote: > > We should probably set a different default for windows. > > We want the timer enabled (true) for windows due to the windows XP bug. I don't > know if other desktop OSes want this change or not. So by default it is left as > true. We will explicitly set it to false for Android. Sorry, I had the boolean value's effect flipped in my head. This seems fine to me. http://codereview.chromium.org/8526006/diff/12020/net/socket/client_socket_po... File net/socket/client_socket_pool_base_unittest.cc (right): http://codereview.chromium.org/8526006/diff/12020/net/socket/client_socket_po... net/socket/client_socket_pool_base_unittest.cc:2000: // We post all of our delayed tasks with a 2ms delay. I.e. they don't On 2011/11/16 01:01:08, selim wrote: > On 2011/11/16 00:51:40, willchan wrote: > > Is this comment true? > > I took this part of the test exactly from the one immediate below. I don't know > if this comment is still valid or not. Looks bogus to me. Please remove it. Explain what the 20ms is for (I assume it's to give a little more buffer than just 10ms from the idle socket timer).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sgurun@google.com/8526006/14008
checked the commit box!
Change committed as 110250 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
