code review 1731047: net: Add timeout support windows implement

net: Add timeout support windows implement.

[vcc, 2010-06-30 08:42:24]

Hello brainman, golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com,
vcc),

I'd like you to review this change.

[vcc, 2010-07-16 04:35:54]

update code, please take another look.

[brainman, 2010-07-19 00:53:08]

I don't think it is going to work. The problem here is that once we submitted
buffer for read / write to os, we must wait for os to finish with our buffer,
before we do anything else with it.

You can't just stop waiting for io to complete, because if you do that and, for
example, free your input buffer, your pending syscall.WSARecv() can still finish
eventually and os will attempt to write to the buffer that has been freed. We
can't have that.


Alex

[vcc, 2010-07-19 13:57:14]

On 2010/07/19 00:53:08, brainman wrote:
> I don't think it is going to work. The problem here is that once we submitted
> buffer for read / write to os, we must wait for os to finish with our buffer,
> before we do anything else with it.
> 
> You can't just stop waiting for io to complete, because if you do that and,
for
> example, free your input buffer, your pending syscall.WSARecv() can still
finish
> eventually and os will attempt to write to the buffer that has been freed.

I think garbage collector should take care of the buffer (passed as
unsafe.Pointer), but I can't find the document about GC details, so if it is a
problem, we can easy set the return buffer to another to avoid this.

Wei guangjing

[brainman, 2010-07-20 00:20:34]

On 2010/07/19 13:57:14, vcc wrote:
> 
> I think garbage collector should take care of the buffer (passed as
> unsafe.Pointer), but I can't find the document about GC details, so if it is a
> problem, we can easy set the return buffer to another to avoid this.
> 

There is no magic. If you expect your programming language to perform magic for
you, you're in the wrong place <g>.


Alex

[vcc, 2010-07-22 06:49:31]

On 2010/07/20 00:20:34, brainman wrote:
> 
> There is no magic. If you expect your programming language to perform magic
for
> you, you're in the wrong place <g>.
> 

Yes, there is no magic, so we need create a magic here <g>. Use a list hold the
buffer until pending syscall.WSARecv() finish to make sure proper free buffer.

Wei guangjing

[brainman, 2010-07-22 06:53:24]

> Yes, there is no magic, so we need create a magic here <g>. Use a list hold
the
> buffer until pending syscall.WSARecv() finish to make sure proper free buffer.

I couldn't come up with a way. Perhaps you could do it. The way your code is at
this moment is not going to work.


Alex

[rsc1, 2010-08-03 20:24:37]

leaving brainman as reviewer until this gets sorted out

[vcc, 2010-11-23 13:49:25]

Rewrite to use time.After, please take another look.

[brainman, 2010-12-01 07:48:52]

On 2010/11/23 13:49:25, vcc wrote:
> Rewrite to use time.After, please take another look.

You might have a solution here. I like it - it is simple.

There is one problem with it. It won't work the way it is now. Just think about
it. You've introduced a goroutine to hold a pointer to the user buffer in case
there is a timeout and you need to return before IO is completed. This will work
well if user releases the buffer right after the timed-out call. But what
happens if user decide to use the buffer for something else? In that instance
you will have both user program and kernel IO routines both reading/writing to
the same block of memory. We can't have that.

But, I think, there is a way. Let's consider a "write" call. If we would use our
own buffer, allocated right before IO call, and then copy returned contents into
user buffer if call succeeded, otherwise return timeout back to the user, and
let our goroutine hold our own buffer until IO call completes. "Read" call could
be implemented in a similar fashion.

I don't like copying data from buffer to buffer, but I see no other way.
Perhaps, we could arrange for copying only occures if socket has non-zero
timeout value.

Alex

[rsc, 2010-12-07 18:38:26]

Windows must have a non-blocking network read
that can be combined with WaitForHandle with a timeout.
The channel solution is nice but it doesn't work when
timeouts happen: if a timeout happens the call is
supposed to return and *not consume any data*
but using the channel solution discards the data
that got read.

[brainman, 2010-12-08 05:49:25]

On 2010/12/07 18:38:26, rsc wrote:
> Windows must have a non-blocking network read
> that can be combined with WaitForHandle with a timeout.

syscall.WSARecv() or syscall.ReadFile() + syscall.WaitForSingleObject() will do
what you're asking (that is very similar to what we do anyway), the problem is
that once WSARecv()/ReadFile() call is started and returns (in pending mode),
you could surely stop waiting at any stage (WaitForSingleObject() or not), but
they will continue to do io into the buffer that you've passed to them whether
you want it or not. Simply speaking, I do not know a reliable/portable way to
tell WSARecv()/ReadFile() to stop/cancel pending io. The only solution I see
here, is to wait indefinitely, until they complete.

> The channel solution is nice but it doesn't work when
> timeouts happen: if a timeout happens the call is
> supposed to return and *not consume any data*
> but using the channel solution discards the data
> that got read.

I know what you're saying, but I have no solution for that problem. What is
proposed here, will surely lose some data if timeouts occur.

Alex

[rsc, 2010-12-08 17:23:43]

I looked some more.

I believe the solution here is to put the sockets into
non-blocking mode via WSAEventSelect and then when
necessary wait for them by using WSAWaitForMultipleEvents.

The code so far has focused on overlapped I/O
not non-blocking I/O.

Russ

[brainman, 2010-12-10 03:29:43]

On 2010/12/08 17:23:43, rsc wrote:
> ... via WSAEventSelect ...

Yes. This might work, because, unlike our existing code, order of actions is
reversed: you wait for IO to be ready first and then you start IO (which is
expected to complete immediately).

Alex

[brainman, 2010-12-23 02:34:01]

On 2010/12/08 05:49:25, brainman wrote:
> On 2010/12/07 18:38:26, rsc wrote:
> 
> > The channel solution is nice but it doesn't work when
> > timeouts happen: if a timeout happens the call is
> > supposed to return and *not consume any data*
> > but using the channel solution discards the data
> > that got read.
> 
> I know what you're saying, but I have no solution for that problem. What is
> proposed here, will surely lose some data if timeouts occur.
> 

On second thought, if we associate a dedicated goroutine to every non zero
timeout connection, then it could assure that all data received is consumed. So,
for example, if this goroutine received some data after timed out call have
returned with error, it could supply the data to the next caller.

Alex

[vcc, 2010-12-23 03:09:59]

2010/12/23 <alex.brainman@gmail.com>

> On 2010/12/08 05:49:25, brainman wrote:
>
>> On 2010/12/07 18:38:26, rsc wrote:
>>
>
>  > The channel solution is nice but it doesn't work when
>> > timeouts happen: if a timeout happens the call is
>> > supposed to return and *not consume any data*
>> > but using the channel solution discards the data
>> > that got read.
>>
>
>  I know what you're saying, but I have no solution for that problem.
>>
> What is
>
>> proposed here, will surely lose some data if timeouts occur.
>>
>
>
> On second thought, if we associate a dedicated goroutine to every non
> zero timeout connection, then it could assure that all data received is
> consumed. So, for example, if this goroutine received some data after
> timed out call have returned with error, it could supply the data to the
> next caller.
>

Yes, it should work.


> http://codereview.appspot.com/1731047/
>

[vcc, 2010-12-29 03:24:53]

Solution with CancelIO, I think it would be final solution, please take another
look.

[brainman, 2010-12-29 04:19:52]

http://codereview.appspot.com/1731047/diff/35001/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

http://codereview.appspot.com/1731047/diff/35001/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:253: runtime.LockOSThread()
I suspect you did that to allow CancelIO to work properly, because as per
CancelIO doco:

"The function does not cancel I/O operations that other threads issue for a file
handle."

That should make CancelIO do its job all right. But pinning OS thread to a
pending network IO, will bring us back to 1 thread per 1 connection model,
something we're trying to overcome here. Maybe it is OK as long as we restrict
it to IO with timeouts only.

I still think we could do it, if we're prepared to use intermediate buffer, as I
suggested earlier.

Alex

[vcc, 2010-12-29 14:23:53]

2010/12/29 <alex.brainman@gmail.com>

>
> http://codereview.appspot.com/1731047/diff/35001/src/pkg/net/fd_windows.go
> File src/pkg/net/fd_windows.go (right):
>
>
>
http://codereview.appspot.com/1731047/diff/35001/src/pkg/net/fd_windows.go#ne...
> src/pkg/net/fd_windows.go:253: runtime.LockOSThread()
> I suspect you did that to allow CancelIO to work properly, because as
> per CancelIO doco:
>
> "The function does not cancel I/O operations that other threads issue
> for a file handle."
>
> That should make CancelIO do its job all right. But pinning OS thread to
> a pending network IO, will bring us back to 1 thread per 1 connection
> model,  ...


This is not true, I don't think LockOSThread will be 1 thread per 1
connection, LockOSThread ensure one goroutine always execute in same thread,
and most code run in main goroutine or a  few goroutines, not 1 thread per 1
connection. I test connect many connection with set timeout, and don't see
any thread number increase.


> ... something we're trying to overcome here. Maybe it is OK as long
> as we restrict it to IO with timeouts only.
>
> I still think we could do it, if we're prepared to use intermediate
> buffer, as I suggested earlier.
>

CancelIO solution is simple, and don't need copy data and manage another
buffers and goroutines, I think would be better choice.


>
>
> http://codereview.appspot.com/1731047/
>

[vcc, 2010-12-29 15:33:31]

2010/12/29 Wei guangjing <vcc.163@gmail.com>

> 2010/12/29 <alex.brainman@gmail.com>
>
>>
>> http://codereview.appspot.com/1731047/diff/35001/src/pkg/net/fd_windows.go
>> File src/pkg/net/fd_windows.go (right):
>>
>>
>>
http://codereview.appspot.com/1731047/diff/35001/src/pkg/net/fd_windows.go#ne...
>> src/pkg/net/fd_windows.go:253: runtime.LockOSThread()
>> I suspect you did that to allow CancelIO to work properly, because as
>> per CancelIO doco:
>>
>> "The function does not cancel I/O operations that other threads issue
>> for a file handle."
>>
>> That should make CancelIO do its job all right. But pinning OS thread to
>> a pending network IO, will bring us back to 1 thread per 1 connection
>> model,  ...
>
>
> This is not true, I don't think LockOSThread will be 1 thread per 1
> connection, LockOSThread ensure one goroutine always execute in same thread,
> and most code run in main goroutine or a  few goroutines, not 1 thread per 1
> connection. I test connect many connection with set timeout, and don't see
> any thread number increase.
>

Sorry, after change test code to one connection start a new goroutine, I see
the thread number increase. Yes, this is a problem, if change to use
CancelIoEx, we don't need LockOSThread, but CancelIoEx only available in
Vista and uper. It look like would need run all timeout IO operation in one
special goroutine to slove this problem.



>> http://codereview.appspot.com/1731047/
>>
>
>

[vcc, 2010-12-31 08:36:32]

run all timeout IO operation in one special goroutine to slove 1 thread 1
connection problem, please take another look.

[brainman, 2011-01-07 05:35:17]

LGTM. Well done.

In my opinion fd_windows.go is a mess. But I'm happy to submit, and then sort
that out.

Thank you.

http://codereview.appspot.com/1731047/diff/43001/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

http://codereview.appspot.com/1731047/diff/43001/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:240: READ     = 0
You don't want these to be visible from outside of net.

http://codereview.appspot.com/1731047/diff/43001/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:240: READ     = 0
iota ?

http://codereview.appspot.com/1731047/diff/43001/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:259: var ioChan chan *arg = make(chan *arg, 100)
Why do you need them buffered?

http://codereview.appspot.com/1731047/diff/43001/src/pkg/syscall/syscall_wind...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/1731047/diff/43001/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:483: //sys	CancelIo(s uint32) (ok bool, errno
int)
It is part of kernel32, it will set better along with other kernel32 functions.
If you could move it somewhere near GetQueuedCompletionStatus, it would be good.
Thank you.

[vcc, 2011-01-10 05:16:15]

Please take another look.

http://codereview.appspot.com/1731047/diff/43001/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

http://codereview.appspot.com/1731047/diff/43001/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:240: READ     = 0
On 2011/01/07 05:35:17, brainman wrote:
> You don't want these to be visible from outside of net.

Done.

http://codereview.appspot.com/1731047/diff/43001/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:240: READ     = 0
On 2011/01/07 05:35:17, brainman wrote:
> iota ?

Done.

http://codereview.appspot.com/1731047/diff/43001/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:259: var ioChan chan *arg = make(chan *arg, 100)
On 2011/01/07 05:35:17, brainman wrote:
> Why do you need them buffered?

Done, don't need buffered. I just feel buffered chan would a bit fast, put item
into chan that don't need waiting.

http://codereview.appspot.com/1731047/diff/43001/src/pkg/syscall/syscall_wind...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/1731047/diff/43001/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:483: //sys	CancelIo(s uint32) (ok bool, errno
int)
On 2011/01/07 05:35:17, brainman wrote:
> It is part of kernel32, it will set better along with other kernel32
functions.
> If you could move it somewhere near GetQueuedCompletionStatus, it would be
good.
> Thank you.

Done.

[rsc, 2011-01-11 17:06:29]

http://codereview.appspot.com/1731047/diff/49001/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

http://codereview.appspot.com/1731047/diff/49001/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:13: "runtime"
sort import list

http://codereview.appspot.com/1731047/diff/49001/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:220: if delta > 0 {
can simplify a bit

if delta <= 0 {
    return <-pckt.c
}

select {
...

http://codereview.appspot.com/1731047/diff/49001/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:261: func doTimeoutIO() {
Should describe why this is here.
Why is it important for the calls to be from a single
OS thread?  Would it work just as well to use a mutex
so they don't overlap, or is the single thread necessary?

also s/doTimeoutIO/timeoutIO/.  do is usually noise.

[brainman, 2011-01-11 23:38:12]

http://codereview.appspot.com/1731047/diff/49001/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

http://codereview.appspot.com/1731047/diff/49001/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:261: func doTimeoutIO() {
On 2011/01/11 17:06:29, rsc wrote:
> Why is it important for the calls to be from a single
> OS thread?  ...

It is all CancelIo fault! From CancelIo manual:

" ... Cancels all pending input and output (I/O) operations that are issued by
the calling thread for the specified file. The function does not cancel I/O
operations that other threads issue for a file handle. ... "

Mind you, I just thought of a problem with our approach. CancelIO will cancel
"all" IO for a socket. We do not have multiple simultaneous reads (all our reads
and writes are serialized). But it is possible to have one read and one write at
the same time - both will be canceled, while the user might want to cancel just
one. I still think we should go ahead with this solution, in the hope that these
scenarios are unlikely: simultaneous read and write on the same socket both
blocked and at least one of them have timeout started.

[vcc, 2011-01-12 04:29:54]

PTAL.

http://codereview.appspot.com/1731047/diff/49001/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

http://codereview.appspot.com/1731047/diff/49001/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:13: "runtime"
On 2011/01/11 17:06:29, rsc wrote:
> sort import list

Done.

http://codereview.appspot.com/1731047/diff/49001/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:220: if delta > 0 {
On 2011/01/11 17:06:29, rsc wrote:
> can simplify a bit
> 
> if delta <= 0 {
>     return <-pckt.c
> }
> 
> select {
> ...

Done.

http://codereview.appspot.com/1731047/diff/49001/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:261: func doTimeoutIO() {
On 2011/01/11 17:06:29, rsc wrote:
> also s/doTimeoutIO/timeoutIO/.  do is usually noise.

Done.

[rsc, 2011-01-19 19:49:05]

LGTM

[rsc, 2011-01-19 19:49:27]

*** Submitted as 55be663c1b9c ***

net: implement windows timeout

R=brainman, rsc
CC=golang-dev
http://codereview.appspot.com/1731047

Committer: Russ Cox <rsc@golang.org>