code review 1600041: net: initial attempt to implement windows version

net: initial attempt to implement windows version

[brainman, 2010-06-08 06:19:52]

Hello golang-dev@googlegroups.com,

I'd like you to review this change.

[rsc1, 2010-06-11 09:03:54]

http://codereview.appspot.com/1600041/diff/2001/3002
File src/pkg/net/fd_windows.go (right):

http://codereview.appspot.com/1600041/diff/2001/3002#newcode15
src/pkg/net/fd_windows.go:15: // BUG(brainman): SetTimeout is not implemented. I
do not know how to.
// BUG(brainmain): The Windows implementation does not implement SetTimeout.

http://codereview.appspot.com/1600041/diff/2001/3002#newcode61
src/pkg/net/fd_windows.go:61: return nil,
os.NewSyscallError("GetQueuedCompletionStatus", e)
CreateIoCompletionPort

http://codereview.appspot.com/1600041/diff/2001/3002#newcode214
src/pkg/net/fd_windows.go:214: // IO completed immediately, but we need to get
our completion message anyway.
Is there some way around this?
it seems unnecessarily expensive.

http://codereview.appspot.com/1600041/diff/2001/3002#newcode297
src/pkg/net/fd_windows.go:297: var lrsa, rrsa *syscall.RawSockaddrAny
RawSockaddrAny is supposed to be big enough
to hold any sockaddr struct.
You should be able to declare

var lrsa, rrsa syscall.RawSockaddrAny

and then just pass &lrsa and &rrsa.

Can you change AcceptEx to take *RawSockaddrAny
instead of the *byte?  Then you woudln't need unsafe here.

http://codereview.appspot.com/1600041/diff/2001/3002#newcode363
src/pkg/net/fd_windows.go:363: panic(syscall.Errstr(e))
I'm not sure this is panic-worthy.  Instead, it should probably
just set a global

var initErr os.Error

and then newFD can check for initErr != nil.

[czaplinski, 2010-06-11 22:46:58]

A "battle report" from a fellow Windows user:
- after applying Patch Set 2 to revision 821ca9bf0ec5 of Go, I got godoc to
build and run succesfully (the docs could be browsed on localhost);
- unfortunately, a simple program with only 'http.Get("http://74.125.43.99")' in
main() crashes on my computer - but I didn't have time to disassemble yet.

[brainman, 2010-06-12 04:35:58]

http://codereview.appspot.com/1600041/diff/2001/3002
File src/pkg/net/fd_windows.go (right):

http://codereview.appspot.com/1600041/diff/2001/3002#newcode15
src/pkg/net/fd_windows.go:15: // BUG(brainman): SetTimeout is not implemented. I
do not know how to.
On 2010/06/11 09:03:54, rsc1 wrote:
> // BUG(brainmain): The Windows implementation does not implement SetTimeout.
> 

Done.

http://codereview.appspot.com/1600041/diff/2001/3002#newcode61
src/pkg/net/fd_windows.go:61: return nil,
os.NewSyscallError("GetQueuedCompletionStatus", e)
On 2010/06/11 09:03:54, rsc1 wrote:
> CreateIoCompletionPort
> 

Done.

http://codereview.appspot.com/1600041/diff/2001/3002#newcode208
src/pkg/net/fd_windows.go:208: pckt.c = make(chan *ioResult)
Russ,

The way WSASend/WSARecv/GetQueuedCompletionStatus work, we could have multiple
goroutines receiving data (Read) at the same time. Not sure, if it is of any
use.

That is why I create pckt.c channel here. But considering this Read is
serialized (fd.rio), maybe I should just pre-make channel in newFD?

http://codereview.appspot.com/1600041/diff/2001/3002#newcode214
src/pkg/net/fd_windows.go:214: // IO completed immediately, but we need to get
our completion message anyway.
On 2010/06/11 09:03:54, rsc1 wrote:
> Is there some way around this?

None that I know of. Perhaps someone else can help here.

> it seems unnecessarily expensive.

It does. But from WSARecv manual:

Return Value

If no error occurs and the receive operation has completed immediately, WSARecv
returns zero. In this case, the completion routine will have already been
scheduled to be called once the calling thread is in the alertable state.

My current code works as is. And I can see that some WSARecv completes with
return code 0, and others with ERROR_IO_PENDING. I do

r := <-pckt.c

for both of those. And all of those waits complete. Therefor all those
completion events are fired.

Maybe there is another way of doing it, but I don't know it.

http://codereview.appspot.com/1600041/diff/2001/3002#newcode297
src/pkg/net/fd_windows.go:297: var lrsa, rrsa *syscall.RawSockaddrAny
On 2010/06/11 09:03:54, rsc1 wrote:
> Can you change AcceptEx to take *RawSockaddrAny
> instead of the *byte?  Then you woudln't need unsafe here.

I can't see how. It's quite ugly in here. The (*byte) buffer is supposed to get
3 things in it: some data from peer (if you want it), local address and remote
address. All in the name of efficiency <g>.

From the manual:

lpOutputBuffer [in]

A pointer to a buffer that receives the first block of data sent on a new
connection, the local address of the server, and the remote address of the
client. The receive data is written to the first part of the buffer starting at
offset zero, while the addresses are written to the latter part of the buffer.
This parameter must be specified.

http://codereview.appspot.com/1600041/diff/2001/3002#newcode363
src/pkg/net/fd_windows.go:363: panic(syscall.Errstr(e))
On 2010/06/11 09:03:54, rsc1 wrote:
> I'm not sure this is panic-worthy.  Instead, it should probably
> just set a global
> 
> var initErr os.Error
> 
> and then newFD can check for initErr != nil.
> 

Done.

[brainman, 2010-06-12 04:36:28]

Hello rsc, Mateusz Czaplinski (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc1, 2010-06-12 17:57:38]

looking good

http://codereview.appspot.com/1600041/diff/2001/3002
File src/pkg/net/fd_windows.go (right):

http://codereview.appspot.com/1600041/diff/2001/3002#newcode208
src/pkg/net/fd_windows.go:208: pckt.c = make(chan *ioResult)
On 2010/06/12 04:35:58, brainman wrote:
> Russ,
> 
> The way WSASend/WSARecv/GetQueuedCompletionStatus work, we could have multiple
> goroutines receiving data (Read) at the same time. Not sure, if it is of any
> use.
> 
> That is why I create pckt.c channel here. But considering this Read is
> serialized (fd.rio), maybe I should just pre-make channel in newFD?

Sounds good.  That's what the Unix implementation does, I think.

http://codereview.appspot.com/1600041/diff/2001/3002#newcode214
src/pkg/net/fd_windows.go:214: // IO completed immediately, but we need to get
our completion message anyway.
On 2010/06/12 04:35:58, brainman wrote:
> On 2010/06/11 09:03:54, rsc1 wrote:
> > Is there some way around this?
> 
> None that I know of. Perhaps someone else can help here.

I agree with your reading of the manual - looks required.
Fine for now.

http://codereview.appspot.com/1600041/diff/2001/3002#newcode297
src/pkg/net/fd_windows.go:297: var lrsa, rrsa *syscall.RawSockaddrAny
On 2010/06/12 04:35:58, brainman wrote:
> On 2010/06/11 09:03:54, rsc1 wrote:
> > Can you change AcceptEx to take *RawSockaddrAny
> > instead of the *byte?  Then you woudln't need unsafe here.
> 
> I can't see how. It's quite ugly in here. The (*byte) buffer is supposed to
get
> 3 things in it: some data from peer (if you want it), local address and remote
> address. All in the name of efficiency <g>.
> 
> From the manual:
> 
> lpOutputBuffer [in]
> 
> A pointer to a buffer that receives the first block of data sent on a new
> connection, the local address of the server, and the remote address of the
> client. The receive data is written to the first part of the buffer starting
at
> offset zero, while the addresses are written to the latter part of the buffer.
> This parameter must be specified.

I suggest making sure that RawSockaddrAny has the extra 16 bytes built in
and then make this:

var rsa [2]syscall.RawSockaddrAny
...
_, e = syscall.AcceptEx(fd.sysfd, s, &rsa, &done, &pckt.o)

That at least gets rid of the need for unsafe in the call
(and many of the conversions, which should be taken care
of inside the syscall).

[brainman, 2010-06-13 05:47:53]

Hello rsc, Mateusz Czaplinski (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2010-06-13 05:48:44]

Hello rsc, Mateusz Czaplinski (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2010-06-13 05:49:18]

http://codereview.appspot.com/1600041/diff/2001/3002
File src/pkg/net/fd_windows.go (right):

http://codereview.appspot.com/1600041/diff/2001/3002#newcode208
src/pkg/net/fd_windows.go:208: pckt.c = make(chan *ioResult)
On 2010/06/12 17:57:38, rsc1 wrote:
> On 2010/06/12 04:35:58, brainman wrote:
> > Russ,
> > 
> > The way WSASend/WSARecv/GetQueuedCompletionStatus work, we could have
multiple
> > goroutines receiving data (Read) at the same time. Not sure, if it is of any
> > use.
> > 
> > That is why I create pckt.c channel here. But considering this Read is
> > serialized (fd.rio), maybe I should just pre-make channel in newFD?
> 
> Sounds good.  That's what the Unix implementation does, I think.

Done.

http://codereview.appspot.com/1600041/diff/2001/3002#newcode214
src/pkg/net/fd_windows.go:214: // IO completed immediately, but we need to get
our completion message anyway.
On 2010/06/12 17:57:38, rsc1 wrote:
> On 2010/06/12 04:35:58, brainman wrote:
> > On 2010/06/11 09:03:54, rsc1 wrote:
> > > Is there some way around this?
> > 
> > None that I know of. Perhaps someone else can help here.
> 
> I agree with your reading of the manual - looks required.
> Fine for now.
> 

Done.

http://codereview.appspot.com/1600041/diff/2001/3002#newcode297
src/pkg/net/fd_windows.go:297: var lrsa, rrsa *syscall.RawSockaddrAny
On 2010/06/12 17:57:38, rsc1 wrote:
> On 2010/06/12 04:35:58, brainman wrote:
> > On 2010/06/11 09:03:54, rsc1 wrote:
> > > Can you change AcceptEx to take *RawSockaddrAny
> > > instead of the *byte?  Then you woudln't need unsafe here.
> > 
> > I can't see how. It's quite ugly in here. The (*byte) buffer is supposed to
> get
> > 3 things in it: some data from peer (if you want it), local address and
remote
> > address. All in the name of efficiency <g>.
> > 
> > From the manual:
> > 
> > lpOutputBuffer [in]
> > 
> > A pointer to a buffer that receives the first block of data sent on a new
> > connection, the local address of the server, and the remote address of the
> > client. The receive data is written to the first part of the buffer starting
> at
> > offset zero, while the addresses are written to the latter part of the
buffer.
> > This parameter must be specified.
> 
> I suggest making sure that RawSockaddrAny has the extra 16 bytes built in
> and then make this:
> 
> var rsa [2]syscall.RawSockaddrAny
> ...
> _, e = syscall.AcceptEx(fd.sysfd, s, &rsa, &done, &pckt.o)
> 
> That at least gets rid of the need for unsafe in the call
> (and many of the conversions, which should be taken care
> of inside the syscall).
> 

Done.

[brainman, 2010-06-30 00:42:33]

Hello rsc, Mateusz Czaplinski (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2010-06-30 00:44:19]

On 2010/06/12 17:57:38, rsc1 wrote:
> looking good
> 

Ping

[rsc1, 2010-06-30 03:22:44]

LGTM

[rsc, 2010-06-30 03:23:42]

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

net: initial attempt to implement windows version

R=rsc, Mateusz Czaplinski
CC=golang-dev
http://codereview.appspot.com/1600041

Committer: Russ Cox <rsc@golang.org>