Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1065563006:
Add code to set TCP keep alive delay on OSX. (Closed)
DescriptionAdd code to set TCP keep alive delay on OSX.
Previously, TCP keep alives were enabled on all
desktop platforms, but the delay was not being set on
OSX, resulting in not sending keep alives every 45
seconds, like on other platforms. This CL adds the
code to do that.
It does the same on iOS, though keep alives aren't
enabled on mobile platforms, anyways.
BUG=468764
Committed: https://crrev.com/7e54f921781f44db1b4cf6002a0d180d37846c58
Cr-Commit-Position: refs/heads/master@{#324319}
Patch Set 1 #Patch Set 2 : Oops #
Total comments: 5
Patch Set 3 : Response to comments #
Total comments: 3
Patch Set 4 : Move comment #Messages
Total messages: 25 (8 generated)
mmenke@chromium.org changed reviewers: + jri@chromium.org
Jana: Are you comfortable reviewing this? Don't think we really have a sockets expert.
On 2015/04/06 16:11:19, mmenke wrote: > Jana: Are you comfortable reviewing this? Don't think we really have a sockets > expert. I should add: Experimentally, I never saw Chromium send any keepalives when visiting www.cnn.com without this CL, and with it, we're sending them as expected. The default keepalive time looks to be 75 seconds. Could be servers in question have a 60 second keepalive time...Or we're just not using the system default, for whatever reason.
On 2015/04/06 16:15:27, mmenke wrote: > On 2015/04/06 16:11:19, mmenke wrote: > > Jana: Are you comfortable reviewing this? Don't think we really have a > sockets > > expert. > > I should add: Experimentally, I never saw Chromium send any keepalives when > visiting http://www.cnn.com without this CL, and with it, we're sending them as > expected. The default keepalive time looks to be 75 seconds. Could be servers > in question have a 60 second keepalive time...Or we're just not using the system > default, for whatever reason. Happy to review this. The usage seems correct, based on the kernel code: 1. SO_KEEPALIVE at http://fxr.watson.org/fxr/source/bsd/kern/uipc_socket.c?v=xnu-2050.18.24#L3036 2. TCP_KEEPALIVE at http://fxr.watson.org/fxr/source/bsd/netinet/tcp_usrreq.c?v=xnu-2050.18.24;im... The defaults are described here: http://fxr.watson.org/fxr/source/bsd/netinet/tcp_timer.h?v=xnu-2050.18.24#L111 The default, before the connection is established, is 75s. After the connection is established and data has been received, the default is much larger than 75s -- it's 2 hours (120*60 seconds)! I have no clue why that constant is so high.
A couple of comments https://codereview.chromium.org/1065563006/diff/20001/net/socket/tcp_socket_l... File net/socket/tcp_socket_libevent.cc (right): https://codereview.chromium.org/1065563006/diff/20001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.cc:60: if (setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, &on, sizeof(on))) { Nit: Perhaps add a comment here to say that this setsockopt is applied towards all platforms? https://codereview.chromium.org/1065563006/diff/20001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.cc:85: #endif Should there be a #else? What are the platforms that are left out? FreeBSD should have the same semantics as MACOSX (I think ... I'll take a look).
https://codereview.chromium.org/1065563006/diff/20001/net/socket/tcp_socket_l... File net/socket/tcp_socket_libevent.cc (right): https://codereview.chromium.org/1065563006/diff/20001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.cc:60: if (setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, &on, sizeof(on))) { On 2015/04/06 23:20:00, Jana wrote: > Nit: Perhaps add a comment here to say that this setsockopt is applied towards > all platforms? Done. https://codereview.chromium.org/1065563006/diff/20001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.cc:85: #endif On 2015/04/06 23:20:00, Jana wrote: > Should there be a #else? What are the platforms that are left out? FreeBSD > should have the same semantics as MACOSX (I think ... I'll take a look). There's also ChromeOS, but I believe that has OS_LINUX defined, given the huge number of "#if defined(OS_LINUX) && !defined(OS_CHROMEOS)" There's also OS_NACL, which I believe also has OS_LINUX set. And then there's OS_FREEBSD, OS_SOLARIS, OS_QNX, OS_OPENBSD. I think figuring out which code each (unsupported) platform should use is beyond the scope of this CL, particularly since we have no trybots, and I'm not going to try and build them all manually. For what it's worth, looks like FreeBSD should use the Linux path: http://svnweb.freebsd.org/base/head/sys/netinet/tcp.h?view=markup#l165 https://codereview.chromium.org/1065563006/diff/40001/net/socket/tcp_socket_l... File net/socket/tcp_socket_libevent.cc (right): https://codereview.chromium.org/1065563006/diff/40001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.cc:70: // Setting the keepalive interval varies by platform. Indent here is weird, but that's what git cl format likes.
lgtm (Repeating comment because I don't know if you saw it earlier in #4: The defaults are described here: http://fxr.watson.org/fxr/source/bsd/netinet/tcp_timer.h?v=xnu-2050.18.24#L111 The default, before the connection is established, is 75s. After the connection is established and data has been received, the default is much larger than 75s -- it's 2 hours (120*60 seconds)! I have no clue why that constant is so high.) One comment inline, but LGTM. https://codereview.chromium.org/1065563006/diff/20001/net/socket/tcp_socket_l... File net/socket/tcp_socket_libevent.cc (right): https://codereview.chromium.org/1065563006/diff/20001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.cc:85: #endif On 2015/04/07 15:28:06, mmenke wrote: > On 2015/04/06 23:20:00, Jana wrote: > > Should there be a #else? What are the platforms that are left out? FreeBSD > > should have the same semantics as MACOSX (I think ... I'll take a look). > > There's also ChromeOS, but I believe that has OS_LINUX defined, given the huge > number of "#if defined(OS_LINUX) && !defined(OS_CHROMEOS)" > > There's also OS_NACL, which I believe also has OS_LINUX set. > > And then there's OS_FREEBSD, OS_SOLARIS, OS_QNX, OS_OPENBSD. I think figuring > out which code each (unsupported) platform should use is beyond the scope of > this CL, particularly since we have no trybots, and I'm not going to try and > build them all manually. > > For what it's worth, looks like FreeBSD should use the Linux path: > http://svnweb.freebsd.org/base/head/sys/netinet/tcp.h?view=markup#l165 I think that the FBSD code is close to the MacOS code, but MacOS collapses some of the options that FBSD needs explicit setting. See the setsockopt code for FBSD: http://fxr.watson.org/fxr/source/netinet/tcp_usrreq.c#L1492 and for MacOS: http://fxr.watson.org/fxr/source/bsd/netinet/tcp_usrreq.c?v=xnu-2050.18.24#L1452 MacOS uses a single timer value for keepalives, when it's turned on, where FBSD uses two different user defined values for the first keepalive and for intervals between keepalives. That said, as you pointed out, this is not for this CL, so I'll stop digging further :-)
Thanks for the review! And I did see your comment about 2 hours, but thanks for repeating it - certainly explains why I didn't see any keepalives when experimenting. On 2015/04/07 19:47:19, Jana wrote: > lgtm > > (Repeating comment because I don't know if you saw it earlier in #4: The > defaults are described here: > http://fxr.watson.org/fxr/source/bsd/netinet/tcp_timer.h?v=xnu-2050.18.24#L111 > The default, before the connection is established, is 75s. After the connection > is established and data has been received, the default is much larger than 75s > -- it's 2 hours (120*60 seconds)! I have no clue why that constant is so high.) > > One comment inline, but LGTM.
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1065563006/40001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2015/04/07 19:59:13, I haz the power (commit-bot) wrote: > No LGTM from a valid reviewer yet. Only full committers are accepted. > Even if an LGTM may have been provided, it was from a non-committer, > _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. Oops... I guess you're not a committer, Jana? Had just assumed you were.
mmenke@chromium.org changed reviewers: + rch@chromium.org
Ryan: Mind reviewing this?
mmenke@chromium.org changed reviewers: + rtenneti@chromium.org
Ramant: Could you review this? (Jana's not a committer, rch is OOO... This is a tiny CL, but I feel like too many CLs end up in front of Eric and Sleevi).
lgtm https://codereview.chromium.org/1065563006/diff/40001/net/socket/tcp_socket_l... File net/socket/tcp_socket_libevent.cc (right): https://codereview.chromium.org/1065563006/diff/40001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.cc:70: // Setting the keepalive interval varies by platform. On 2015/04/07 15:28:06, mmenke wrote: > Indent here is weird, but that's what git cl format likes. Is it possible to move the comment inside #ifdef to get around git cl format issue.
mmenke@chromium.org changed reviewers: - rch@chromium.org
Thanks! https://codereview.chromium.org/1065563006/diff/40001/net/socket/tcp_socket_l... File net/socket/tcp_socket_libevent.cc (right): https://codereview.chromium.org/1065563006/diff/40001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.cc:70: // Setting the keepalive interval varies by platform. On 2015/04/08 20:08:38, ramant wrote: > On 2015/04/07 15:28:06, mmenke wrote: > > Indent here is weird, but that's what git cl format likes. > > Is it possible to move the comment inside #ifdef to get around git cl format > issue. Done.
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rtenneti@chromium.org, jri@chromium.org Link to the patchset: https://codereview.chromium.org/1065563006/#ps60001 (title: "Move comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1065563006/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7e54f921781f44db1b4cf6002a0d180d37846c58 Cr-Commit-Position: refs/heads/master@{#324319} |
