code review 11130044: net: implement SetKeepALiveIdlePeriod and SetKeepAliveI...

net: add SetKeepAlivePeriod

Sets both the duration from the last data packet to the first
keep alive packet and the duration between keep alive packets to be
the passed duration.

I've tested the function on both Darwin (10.8.4) and 4.2 Linux.
I've compiled (make.bash) for all the OS's and tested (all.bash)
on Darwin and Linux.

[p, 2013-07-10 22:12:22]

Hello golang-dev@googlegroups.com (cc: veyron-team@google.com),

I'd like you to review this change to
https://code.google.com/p/go

[dave_cheney.net, 2013-07-11 01:32:36]

On 2013/07/10 22:12:22, p wrote:
> Hello mailto:golang-dev@googlegroups.com (cc: mailto:veyron-team@google.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go

Thank you for your change. 

Please fix the description, should be something like

net: add SetKeepAliveIdlePeriod and SetKeepAliveInterval

Have you discussed this change on the mailing list first ?

[rsc, 2013-07-11 02:06:10]

Do you intend to set them differently? It seems like we could have one call that
did both.

[dvyukov, 2013-07-15 14:27:57]

Please Reply All (including codereview bot), otherwise comments are
not visible on codereview:
https://codereview.appspot.com/11130044/

On Sat, Jul 13, 2013 at 3:18 AM, David Presotto
<presotto+facebook@gmail.com> wrote:
> PTAL
>
> On Thu, Jul 11, 2013 at 1:53 PM, David Presotto <presotto@gmail.com> wrote:
>>
>> I fixed the change description and made Russ's suggested changes, i.e.,
>> make one call that works the same everywhere setting both parameters to the
>> same number AND make the arg a time.Duration instead of an integer seconds.
>>
>>
>> On Wed, Jul 10, 2013 at 8:11 PM, Russ Cox <rsc@golang.org> wrote:
>>>
>>> Thanks. It should probably take a time.Duration, not an integer.

[presotto+facebook_gmail.com, 2013-07-15 15:18:47]

Changed the description and made rsc's suggested changes, i.e., changed
secs to a time.Duration and made it a single call that sets both parameters
to the same value.


On Wed, Jul 10, 2013 at 6:32 PM, <dave@cheney.net> wrote:

> On 2013/07/10 22:12:22, p wrote:
>
>> Hello mailto:golang-dev@**googlegroups.com <golang-dev@googlegroups.com>(cc:
>>
> mailto:veyron-team@google.com)**,
>
>
>  I'd like you to review this change to
>> https://code.google.com/p/go
>>
>
> Thank you for your change.
>
> Please fix the description, should be something like
>
> net: add SetKeepAliveIdlePeriod and SetKeepAliveInterval
>
> Have you discussed this change on the mailing list first ?
>
>
>
>
https://codereview.appspot.**com/11130044/<https://codereview.appspot.com/111...
>

[rsc, 2013-07-15 20:37:55]

LGTM

[nkatsaros, 2013-07-15 21:50:24]

Is it really a good idea to set the keepalive time and interval to the same 
value?

On Linux, by default the first probe is sent 7200 seconds after the last 
data and then every 75 seconds. On Windows, the values are 7200 seconds and 
1 second.

I think most people want to decrease the amount of time to the first 
keepalive probe to something below two hours, but want the interval to stay 
the OS default or something short. If this API was used and the period was 
changed to, say, 30 minutes, the connection would take quite long to drop 
because the OS needs to send many probes. Assuming the OS sends 8 probes, a 
connection with these settings would take 4 hours to die versus 2h10m with 
the OS defaults.

OSX and BSD don't seem to have the ability to set the interval of an 
individual socket so I don't know if using TCP_KEEPALIVE changes that 
parameter.

Given these circumstances I would prefer that the interval is not changed 
at all even where it is supported and only the keepalive time is changed.

[rsc, 2013-07-15 22:40:58]

*** Submitted as https://code.google.com/p/go/source/detail?r=2eed959dd4c7 ***

net: add SetKeepAlivePeriod

Sets both the duration from the last data packet to the first
keep alive packet and the duration between keep alive packets to be
the passed duration.

I've tested the function on both Darwin (10.8.4) and 4.2 Linux.
I've compiled (make.bash) for all the OS's and tested (all.bash)
on Darwin and Linux.

R=golang-dev, dave, rsc, dvyukov, presotto+facebook, nick
CC=golang-dev, veyron-team
https://codereview.appspot.com/11130044

Committer: Russ Cox <rsc@golang.org>