code review 11393043: net: add SetKeepAlivePeriod for windows

net: add SetKeepAlivePeriod for windows

[nkatsaros, 2014-01-07 22:46:56]

Hello golang-codereviews@googlegroups.com,

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

[brainman, 2014-01-07 23:56:05]

No new test?

Alex

https://codereview.appspot.com/11393043/diff/28001/src/pkg/net/tcpsockopt_win...
File src/pkg/net/tcpsockopt_windows.go (right):

https://codereview.appspot.com/11393043/diff/28001/src/pkg/net/tcpsockopt_win...
src/pkg/net/tcpsockopt_windows.go:32: return os.NewSyscallError("WSAIoctl",
syscall.WSAIoctl(fd.sysfd, syscall.SIO_KEEPALIVE_VALS,
(*byte)(unsafe.Pointer(&ka)), size, nil, 0, &ret, nil, 0))
Please, put call to syscall.WSAIoctl on a separate line. Makes it easier to
read.

[nkatsaros, 2014-01-08 16:54:42]

PTAL

I've tested this on Windows 7 x64. I don't really know how to check that it's
working other than looking at the keepalive replies using wireshark. The
original commit added SetKeepAlivePeriod support 11130044 did not have a test.

https://codereview.appspot.com/11393043/diff/28001/src/pkg/net/tcpsockopt_win...
File src/pkg/net/tcpsockopt_windows.go (right):

https://codereview.appspot.com/11393043/diff/28001/src/pkg/net/tcpsockopt_win...
src/pkg/net/tcpsockopt_windows.go:32: return os.NewSyscallError("WSAIoctl",
syscall.WSAIoctl(fd.sysfd, syscall.SIO_KEEPALIVE_VALS,
(*byte)(unsafe.Pointer(&ka)), size, nil, 0, &ret, nil, 0))
On 2014/01/07 23:56:05, brainman wrote:
> Please, put call to syscall.WSAIoctl on a separate line. Makes it easier to
> read.

Done

[bradfitz, 2014-01-08 17:35:36]

LGTM at least.

MSDN says SIO_KEEPALIVE_VALS has been supported since Win2k, so no need for
conditional syscalls or anything.

Leaving for Alex to submit once he's happy.  I agree a test would be nice
but I don't see how.  Maybe get the value back and see if it matches.  MSDN
sasys "To query whether TCP keep-alive is enabled on a socket, the
getsockopt function can be called with the SO_KEEPALIVE option.".  (I don't
propose adding new public API, of course... just in a test)



On Wed, Jan 8, 2014 at 8:54 AM, <nick@nickkatsaros.com> wrote:

> PTAL
>
> I've tested this on Windows 7 x64. I don't really know how to check that
> it's working other than looking at the keepalive replies using
> wireshark. The original commit added SetKeepAlivePeriod support 11130044
> did not have a test.
>
>
>
> https://codereview.appspot.com/11393043/diff/28001/src/
> pkg/net/tcpsockopt_windows.go
> File src/pkg/net/tcpsockopt_windows.go (right):
>
> https://codereview.appspot.com/11393043/diff/28001/src/
> pkg/net/tcpsockopt_windows.go#newcode32
> src/pkg/net/tcpsockopt_windows.go:32: return
> os.NewSyscallError("WSAIoctl", syscall.WSAIoctl(fd.sysfd,
> syscall.SIO_KEEPALIVE_VALS, (*byte)(unsafe.Pointer(&ka)), size, nil, 0,
> &ret, nil, 0))
> On 2014/01/07 23:56:05, brainman wrote:
>
>> Please, put call to syscall.WSAIoctl on a separate line. Makes it
>>
> easier to
>
>> read.
>>
>
> Done
>
>
> https://codereview.appspot.com/11393043/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[brainman, 2014-01-09 06:19:05]

LGTM.

I admit, I don't know how to test it myself. How did you discover that this
function is unimplemented?

It seems this is your first contribution. You have to complete one of the
contributor license agreements, as described in
http://golang.org/doc/contribute.html#copyright. Ping us here once done, and one
of us will submit your CL.

Thank you.

Alex

[nkatsaros, 2014-01-09 07:31:25]

I followed the original CL for this feature because it was something I needed.
It was already implemented in my project so I made this change to add windows
support and forgot about mailing it until I saw
https://groups.google.com/forum/#!topic/golang-dev/zh6frmUA8no.

I signed the individual CLA yesterday.

Nick

[brainman, 2014-01-09 09:39:07]

On 2014/01/09 07:31:25, nkatsaros wrote:
> ...
> I signed the individual CLA yesterday.

Brad, can you, please, look for it. Thank you.

Alex

[bradfitz, 2014-01-09 17:44:03]

On Thu, Jan 9, 2014 at 1:39 AM, <alex.brainman@gmail.com> wrote:

> On 2014/01/09 07:31:25, nkatsaros wrote:
>
>> ...
>>
>> I signed the individual CLA yesterday.
>>
>
> Brad, can you, please, look for it. Thank you.
>

Done.

[mikio, 2014-01-10 01:44:10]

On Thu, Jan 9, 2014 at 1:54 AM,  <nick@nickkatsaros.com> wrote:

> I've tested this on Windows 7 x64. I don't really know how to check that
> it's working other than looking at the keepalive replies using
> wireshark. The original commit added SetKeepAlivePeriod support 11130044
> did not have a test.

No worries. There's no easy way to test "dead TCP peer detection"
feature except having an IP intermediate system that can intercept
target TCP control plane. Perhaps we'll have such environment consists
of a few testbots over the wildnet but not now.

[brainman, 2014-01-10 03:34:00]

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

net: add SetKeepAlivePeriod for windows

R=golang-codereviews, alex.brainman, bradfitz, mikioh.mikioh
CC=golang-codereviews
https://codereview.appspot.com/11393043

Committer: Alex Brainman <alex.brainman@gmail.com>

[brainman, 2014-01-10 03:34:29]

On 2014/01/09 17:44:03, bradfitz wrote:
> 
> Done.

Thank you.

Alex