code review 6604072: net: fix connection resets when closed on windows

net: fix connection resets when closed on windows

It is common to close network connection while another goroutine is
blocked reading on another goroutine. This sequence corresponds to
windows calls to WSARecv to start io, followed by GetQueuedCompletionStatus
that blocks until io completes, and, finally, closesocket called from
another thread. We were expecting that closesocket would unblock
GetQueuedCompletionStatus, and it does, but not always
(http://code.google.com/p/go/issues/detail?id=4170#c5). Also that sequence
results in connection is being reset.

This CL inserts CancelIo between GetQueuedCompletionStatus and closesocket,
and waits for both WSARecv and GetQueuedCompletionStatus to complete before
proceeding to closesocket.  This seems to fix both connection resets and
issue 4170. It also makes windows code behave similar to unix version.

Unfortunately, CancelIo needs to be called on the same thread as WSARecv.
So we have to employ strategy we use for connections with deadlines to
every connection now. It means, there are 2 unavoidable thread switches
for every io. Some newer versions of windows have new CancelIoEx api that
doesn't have these drawbacks, and this CL uses this capability when available.
As time goes by, we should have less of CancelIo and more of CancelIoEx
systems. Computers with CancelIoEx are also not affected by issue 4195 anymore.

Fixes issue 3710
Fixes issue 3746
Fixes issue 4170
Partial fix for issue 4195

[brainman, 2012-10-11 06:28:09]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[mikio, 2012-10-13 17:13:50]

It might be better syscall for windows stuff in another CL, perhaps.

https://codereview.appspot.com/6604072/diff/5001/src/pkg/net/fd_unix.go
File src/pkg/net/fd_unix.go (right):

https://codereview.appspot.com/6604072/diff/5001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:674: var skipReadWriteDeadlineTest bool // used for
testing current package
s/skipReadWriteDeadlineTest/canCancelIO/
also pls move this line before init() as same as fd_windows.go.

https://codereview.appspot.com/6604072/diff/5001/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

https://codereview.appspot.com/6604072/diff/5001/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:21: var skipReadWriteDeadlineTest bool // used for
testing current package
we can drop this variable.

https://codereview.appspot.com/6604072/diff/5001/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:139: // Is used on Windows with no CancelIoEx api
only.
It's?

https://codereview.appspot.com/6604072/diff/5001/src/pkg/net/timeout_test.go
File src/pkg/net/timeout_test.go (right):

https://codereview.appspot.com/6604072/diff/5001/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:124: if skipReadWriteDeadlineTest {
if !canCancelIO {

[brainman, 2012-10-15 04:28:09]

Hello golang-dev@googlegroups.com, mikioh.mikioh@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[brainman, 2012-10-15 04:29:22]

https://codereview.appspot.com/6604072/diff/5001/src/pkg/net/fd_unix.go
File src/pkg/net/fd_unix.go (right):

https://codereview.appspot.com/6604072/diff/5001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:674: var skipReadWriteDeadlineTest bool // used for
testing current package
On 2012/10/13 17:13:50, mikio wrote:
> s/skipReadWriteDeadlineTest/canCancelIO/
> also pls move this line before init() as same as fd_windows.go.

Done.

https://codereview.appspot.com/6604072/diff/5001/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

https://codereview.appspot.com/6604072/diff/5001/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:21: var skipReadWriteDeadlineTest bool // used for
testing current package
On 2012/10/13 17:13:50, mikio wrote:
> we can drop this variable.

Done.

https://codereview.appspot.com/6604072/diff/5001/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:139: // Is used on Windows with no CancelIoEx api
only.
On 2012/10/13 17:13:50, mikio wrote:
> It's?

Done.

https://codereview.appspot.com/6604072/diff/5001/src/pkg/net/timeout_test.go
File src/pkg/net/timeout_test.go (right):

https://codereview.appspot.com/6604072/diff/5001/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:124: if skipReadWriteDeadlineTest {
On 2012/10/13 17:13:50, mikio wrote:
> if !canCancelIO {

Done.

[bradfitz, 2012-10-16 03:46:42]

https://codereview.appspot.com/6604072/diff/11002/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

https://codereview.appspot.com/6604072/diff/11002/src/pkg/net/fd_windows.go#n...
src/pkg/net/fd_windows.go:137: // It is used on Windows with no CancelIoEx api
only.
API.  Also, I'd reword:

// It is used on Windows only when the CancelIoEx API is unavailable.

https://codereview.appspot.com/6604072/diff/11002/src/pkg/net/fd_windows.go#n...
src/pkg/net/fd_windows.go:210: // Wait for io to be canceled or complete
successfully.
IO (capital), like the next line.  (and elsewhere)

https://codereview.appspot.com/6604072/diff/11002/src/pkg/net/fd_windows.go#n...
src/pkg/net/fd_windows.go:238: // Windows api determines the outcome.
API.

I would explain a little bit more here.  As is, it reads like "the following
line uses an if statement".  You have a great CL description but those
paragraphs of text will be harder to find once submitted.  It'd be great if some
of that made its way into the code somewhere.

https://codereview.appspot.com/6604072/diff/11002/src/pkg/net/fd_windows.go#n...
src/pkg/net/fd_windows.go:278: closec: make(chan bool, 2), // Close shouldn't
block when signalling to both reader and writer
why not just close the channel to signal close?

[brainman, 2012-10-16 06:58:00]

Hello golang-dev@googlegroups.com, mikioh.mikioh@gmail.com, bradfitz@golang.org
(cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2012-10-16 06:58:09]

https://codereview.appspot.com/6604072/diff/11002/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

https://codereview.appspot.com/6604072/diff/11002/src/pkg/net/fd_windows.go#n...
src/pkg/net/fd_windows.go:137: // It is used on Windows with no CancelIoEx api
only.
On 2012/10/16 03:46:42, bradfitz wrote:
> API.  Also, I'd reword:
> 
> // It is used on Windows only when the CancelIoEx API is unavailable.

Done.

https://codereview.appspot.com/6604072/diff/11002/src/pkg/net/fd_windows.go#n...
src/pkg/net/fd_windows.go:210: // Wait for io to be canceled or complete
successfully.
On 2012/10/16 03:46:42, bradfitz wrote:
> IO (capital), like the next line.  (and elsewhere)

Done.

https://codereview.appspot.com/6604072/diff/11002/src/pkg/net/fd_windows.go#n...
src/pkg/net/fd_windows.go:238: // Windows api determines the outcome.
On 2012/10/16 03:46:42, bradfitz wrote:
> API.
> 
> I would explain a little bit more here.  As is, it reads like "the following
> line uses an if statement".  You have a great CL description but those
> paragraphs of text will be harder to find once submitted.  It'd be great if
some
> of that made its way into the code somewhere.

Done.

https://codereview.appspot.com/6604072/diff/11002/src/pkg/net/fd_windows.go#n...
src/pkg/net/fd_windows.go:278: closec: make(chan bool, 2), // Close shouldn't
block when signalling to both reader and writer
On 2012/10/16 03:46:42, bradfitz wrote:
> why not just close the channel to signal close?

Done.

[rsc, 2012-10-16 17:41:44]

I'm happy once Mikio is.

[brainman, 2012-10-25 00:37:49]

ping

[bradfitz, 2012-10-25 00:46:08]

Waiting on Mikio's review?

On Wed, Oct 24, 2012 at 5:37 PM, <alex.brainman@gmail.com> wrote:

> ping
>
>
http://codereview.appspot.com/**6604072/<http://codereview.appspot.com/6604072/>
>

[brainman, 2012-10-25 00:46:47]

On 2012/10/25 00:46:08, bradfitz wrote:
> Waiting on Mikio's review?
> 

I am.

Alex

[mikio, 2012-10-30 06:08:11]

LGTM

sorry for the late response.

[brainman, 2012-10-30 23:24:43]

*** Submitted as http://code.google.com/p/go/source/detail?r=8087f34d11d6 ***

net: fix connection resets when closed on windows

It is common to close network connection while another goroutine is
blocked reading on another goroutine. This sequence corresponds to
windows calls to WSARecv to start io, followed by GetQueuedCompletionStatus
that blocks until io completes, and, finally, closesocket called from
another thread. We were expecting that closesocket would unblock
GetQueuedCompletionStatus, and it does, but not always
(http://code.google.com/p/go/issues/detail?id=4170#c5). Also that sequence
results in connection is being reset.

This CL inserts CancelIo between GetQueuedCompletionStatus and closesocket,
and waits for both WSARecv and GetQueuedCompletionStatus to complete before
proceeding to closesocket.  This seems to fix both connection resets and
issue 4170. It also makes windows code behave similar to unix version.

Unfortunately, CancelIo needs to be called on the same thread as WSARecv.
So we have to employ strategy we use for connections with deadlines to
every connection now. It means, there are 2 unavoidable thread switches
for every io. Some newer versions of windows have new CancelIoEx api that
doesn't have these drawbacks, and this CL uses this capability when available.
As time goes by, we should have less of CancelIo and more of CancelIoEx
systems. Computers with CancelIoEx are also not affected by issue 4195 anymore.

Fixes issue 3710
Fixes issue 3746
Fixes issue 4170
Partial fix for issue 4195

R=golang-dev, mikioh.mikioh, bradfitz, rsc
CC=golang-dev
http://codereview.appspot.com/6604072