[go] net/rawconn: implement RawRead/RawWrite on Windows
Aman Gupta (Gerrit)
Aman Gupta would like Alex Brainman to review this change.
net/rawconn: implement RawRead/RawWrite on Windows
This implementation was copied from fd_unix.go
Change-Id: Ib10074e9d502c040294f41a260e561e84208652f
---
M src/internal/poll/fd_windows.go
M src/net/rawconn.go
2 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/src/internal/poll/fd_windows.go b/src/internal/poll/fd_windows.go
index 67a4c50..6ae76d9 100644
--- a/src/internal/poll/fd_windows.go
+++ b/src/internal/poll/fd_windows.go
@@ -891,10 +891,38 @@
// RawRead invokes the user-defined function f for a read operation.
func (fd *FD) RawRead(f func(uintptr) bool) error {
- return errors.New("not implemented")
+ if err := fd.readLock(); err != nil {
+ return err
+ }
+ defer fd.readUnlock()
+ if err := fd.pd.prepareRead(fd.isFile); err != nil {
+ return err
+ }
+ for {
+ if f(uintptr(fd.Sysfd)) {
+ return nil
+ }
+ if err := fd.pd.waitRead(fd.isFile); err != nil {
+ return err
+ }
+ }
}
// RawWrite invokes the user-defined function f for a write operation.
func (fd *FD) RawWrite(f func(uintptr) bool) error {
- return errors.New("not implemented")
+ if err := fd.writeLock(); err != nil {
+ return err
+ }
+ defer fd.writeUnlock()
+ if err := fd.pd.prepareWrite(fd.isFile); err != nil {
+ return err
+ }
+ for {
+ if f(uintptr(fd.Sysfd)) {
+ return nil
+ }
+ if err := fd.pd.waitWrite(fd.isFile); err != nil {
+ return err
+ }
+ }
}
diff --git a/src/net/rawconn.go b/src/net/rawconn.go
index 2399c9f..11f01ff 100644
--- a/src/net/rawconn.go
+++ b/src/net/rawconn.go
@@ -9,9 +9,6 @@
"syscall"
)
-// BUG(mikio): On Windows, the Read and Write methods of
-// syscall.RawConn are not implemented.
-
// BUG(mikio): On NaCl and Plan 9, the Control, Read and Write methods
// of syscall.RawConn are not implemented.
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Can we now make net/rawconn_windows_test.go look more like net/rawconn_unix_test.go, including tests of Read and Write?
Aman Gupta (Gerrit)
Aman Gupta uploaded patch set #2 to this change.
net/rawconn: implement RawRead/RawWrite on Windows
This implementation was copied from fd_unix.go
Change-Id: Ib10074e9d502c040294f41a260e561e84208652f
---
M src/internal/poll/fd_windows.go
M src/net/rawconn.go
M src/net/rawconn_windows_test.go
3 files changed, 122 insertions(+), 5 deletions(-)
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.
Aman Gupta (Gerrit)
Patch Set 1:
Can we now make net/rawconn_windows_test.go look more like net/rawconn_unix_test.go, including tests of Read and Write?
Sure, test added in new patch set.
Alex Brainman (Gerrit)
LGTM
Leaving for mikio, because I do not know enough about this area.
Alex
Patch set 3:Code-Review +1
1 comment:
Patch Set #2, Line 7:
net/rawconns|net/rawconn|net|
because the package is called net, not net/rawconn.
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.
Aman Gupta (Gerrit)
Aman Gupta uploaded patch set #4 to this change.
net: implement (*syscall.RawConn).Read/Write on Windows
This implementation was copied from fd_unix.go
Change-Id: Ib10074e9d502c040294f41a260e561e84208652f
---
M src/internal/poll/fd_windows.go
M src/net/rawconn.go
M src/net/rawconn_windows_test.go
3 files changed, 122 insertions(+), 5 deletions(-)
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.
Aman Gupta (Gerrit)
As I discovered in https://go-review.googlesource.com/c/net/+/76394/7#message-c83a2faa09c08b41ea3815c0f0776a12f782417e, this implementation is not correct.
I had copied it from fd_unix.go, but on Windows we need to make use of ExecIO()
However, to actually use IOCP, the RawConn Read/Write callback would need to receive both *Handle and *Overlapped. But the RawConn interface only provides for one argument:
type RawConn interface {
Read(f func(fd uintptr) (done bool)) errorI'm not really sure how to proceed from here..
Aman Gupta (Gerrit)
I'm not really sure how to proceed from here..
For the purposes of golang/go#20153, one option is to leave RawConn.Read/Write unimplemented on Windows. Instead merge CL 76393 so ReadMsg/WriteMsg work, and then have x/net use those on Windows directly in CL 76394 (instead of using RawConn).
Alex Brainman (Gerrit)
However, to actually use IOCP, the RawConn Read/Write callback
would need to receive both *Handle and *Overlapped. But the RawConn
interface only provides for one argument:type RawConn interface {
Read(f func(fd uintptr) (done bool)) error
Correct.
I'm not really sure how to proceed from here..
The only suggestion I have is not to use IOCP. If you are OK to waste thread on each IO, then this is OK.
Aman Gupta (Gerrit)
The only suggestion I have is not to use IOCP. If you are OK to waste thread on each IO, then this is OK.
I'm fine with not using IOCP. But the ICMP tests in x/net expect to set a read deadline, and are failing.
Should I just leave those tests disabled?
Or should I try to emulate timeouts using SO_RCVTIMEO? Is there even a way to get the current read/write deadline for a socket from the runtime?
Alex Brainman (Gerrit)
I'm fine with not using IOCP.
I am not. I think our package API should not prevent us from using capabilities OS provides.
But the ICMP tests in x/net expect to
set a read deadline, and are failing.Should I just leave those tests disabled?
I do not know. I will let Mikio decide.
Or should I try to emulate timeouts using SO_RCVTIMEO?
You could try. But I suspect you will be wasting even more time with this.
Is there
even a way to get the current read/write deadline for a socket from
the runtime?
I am just guessing, but it will be stored somewhere in netpoller. For example, there is runtime_pollSetDeadline function that stores timeout values.
Sorry I am not very helpful.
Alex
Mikio Hara (Gerrit)
thanks for tacking this.
fyi: https://go-review.googlesource.com/c/go/+/37913/1/src/syscall/net.go#9
i think that it's better to discuss windows support on syscall.rawconn first because as described in the cl, support for windows was postponed. i guess that there are a few options including API change; for example adding a method that returns windows event i/o-specific stuff to syscall.rawconn interface on windows.
Mikio Hara (Gerrit)
i think that it's better to discuss windows support on syscall.rawconn first
i mean, at golang-dev, though looks like this cl missed the go1.10 train (just finished to read golang-dev and found the post "code freeze".)
Aman Gupta (Gerrit)
fyi: https://go-review.googlesource.com/c/go/+/37913/1/src/syscall/net.go#9
Thanks for the context. An interface like the one proposed originally would definitely solve the issue here.
BTW, is it possible to change the RawConn interface, or is it frozen now for backwards-compatibility?
i think that it's better to discuss windows support on syscall.rawconn first because as described in the cl, support for windows was postponed. i guess that there are a few options including API change; for example adding a method that returns windows event i/o-specific stuff to syscall.rawconn interface on windows.
I agree it would be best to figure out how to properly support windows here. Please cc me on any discussions, and let me know if you'd like help with the implementation once a proposal is finalized.
i mean, at golang-dev, though looks like this cl missed the go1.10 train (just finished to read golang-dev and found the post "code freeze".)
Bummer, but I understand this must wait for go1.11 now.
Mikio Hara (Gerrit)
BTW, is it possible to change the RawConn interface, or is it frozen now for backwards-compatibility?
there's https://golang.org/doc/go1compat, but it's crystal clear that the existing syscall.rawconn is half-baked stuff and doesn't work on windows. in general, any bug fix is acceptable even if it brings api change, of course you need to convince people using their time for maintaining stdlib. so it depends on your proposal.
Ian Lance Taylor (Gerrit)
As I discovered in https://go-review.googlesource.com/c/net/+/76394/7#message-c83a2faa09c08b41ea3815c0f0776a12f782417e, this implementation is not correct.
I had copied it from fd_unix.go, but on Windows we need to make use of ExecIO()
However, to actually use IOCP, the RawConn Read/Write callback would need to receive both *Handle and *Overlapped. But the RawConn interface only provides for one argument:
type RawConn interface {
Read(f func(fd uintptr) (done bool)) errorI'm not really sure how to proceed from here..
It seems to me that you can use the fd to look up the poll descriptor, and then you can construct your own Overlapped structure.
Aman Gupta (Gerrit)
I'm not really sure how to proceed from here..
It seems to me that you can use the fd to look up the poll descriptor
Is this possible from x/net? Looks like pollDesc is internal.
>, and then you can construct your own Overlapped structure.
The Overlapped struct is only useful in the context of ExecIO, which is also internal and cannot be invoked from x/net.
Ian Lance Taylor (Gerrit)
I'm not really sure how to proceed from here..
It seems to me that you can use the fd to look up the poll descriptor
Is this possible from x/net? Looks like pollDesc is internal.
>, and then you can construct your own Overlapped structure.
The Overlapped struct is only useful in the context of ExecIO, which is also internal and cannot be invoked from x/net.
It is quite possible that I misunderstand. But as far as I can see, the code that needs to do this is RawRead in internal/poll. The only thing you need to do in x/net is call `syscall.WSARecv` or whatever.
Aman Gupta (Gerrit)
It is quite possible that I misunderstand. But as far as I can see, the code that needs to do this is RawRead in internal/poll. The only thing you need to do in x/net is call `syscall.WSARecv` or whatever.
So for instance, currently ReadFrom looks like this (where o.o is the Overlapped struct):
o := &fd.rop
o.Init()
rsrv.ExecIO(o, func(o *operation) error {
return syscall.WSARecvFrom(o.fd.Sysfd, ..., &o.o)
})
The Overlapped struct is created and owned by the operation, because ExecIO needs to be able to use it to cancel the I/O in the event of a timeout.
With the existing RawConn interface, this translates roughly to:
rawConn, _ := conn.Sysconn()
rawConn.Read(func(s uintptr) bool {
return syscall.WSARecvFrom(syscall.Handle(s), ..., ???) != syscall.ERROR_IN_PROGRESS
})
But since only the handle is passed in, we don't have the IOCP context to pass to the syscall. Creating another Overlapped here ourselves doesn't work- it needs to be the one that's tied to the operation and used by ExecIO.
Aman Gupta (Gerrit)
Patch Set 6:
It is quite possible that I misunderstand. But as far as I can see, the code that needs to do this is RawRead in internal/poll. The only thing you need to do in x/net is call `syscall.WSARecv` or whatever.
So for instance, currently ReadFrom looks like this (where o.o is the Overlapped struct):
o := &fd.rop
o.Init()
rsrv.ExecIO(o, func(o *operation) error {
return syscall.WSARecvFrom(o.fd.Sysfd, ..., &o.o)
})The Overlapped struct is created and owned by the operation, because ExecIO needs to be able to use it to cancel the I/O in the event of a timeout.
It's actually used for more than just cancelling. Also for getting the result of the operation.
With the existing RawConn interface, this translates roughly to:rawConn, _ := conn.Sysconn()
rawConn.Read(func(s uintptr) bool {
return syscall.WSARecvFrom(syscall.Handle(s), ..., ???) != syscall.ERROR_IN_PROGRESS
})But since only the handle is passed in, we don't have the IOCP context to pass to the syscall. Creating another Overlapped here ourselves doesn't work- it needs to be the one that's tied to the operation and used by ExecIO.
In fact, the bool in `func(s uintptr) bool` is also problematic. We need to be able to return the actual error from the syscall, and let ExecIO deal with both the ERROR_IN_PROGRESS case as well as immediate errors. Return false on real errors here will just cause the syscall to be attempted over and over forever.
In short, the RawConn interface is not really compatible with IOCP on Windows and needs some serious reconsideration.
The alternative is to merge this CL as-is, which means that RawConn will only allow for blocking reads/writes and won't integrate with IOCP. However, this means that read/write deadlines are not respected (as seen by the test hangs in CL 76394)
Ian Lance Taylor (Gerrit)
It is quite possible that I misunderstand. But as far as I can see, the code that needs to do this is RawRead in internal/poll. The only thing you need to do in x/net is call `syscall.WSARecv` or whatever.
So for instance, currently ReadFrom looks like this (where o.o is the Overlapped struct):
o := &fd.rop
o.Init()
rsrv.ExecIO(o, func(o *operation) error {
return syscall.WSARecvFrom(o.fd.Sysfd, ..., &o.o)
})The Overlapped struct is created and owned by the operation, because ExecIO needs to be able to use it to cancel the I/O in the event of a timeout.
With the existing RawConn interface, this translates roughly to:
rawConn, _ := conn.Sysconn()
rawConn.Read(func(s uintptr) bool {
return syscall.WSARecvFrom(syscall.Handle(s), ..., ???) != syscall.ERROR_IN_PROGRESS
})But since only the handle is passed in, we don't have the IOCP context to pass to the syscall. Creating another Overlapped here ourselves doesn't work- it needs to be the one that's tied to the operation and used by ExecIO.
Ah, I think I see what you mean. We could provide an API for code to dig up the Overlapped context given the descriptor, but it does seem rather awkward to require that in every case. So the current RawConn interface doesn't support overlapped I/O on Windows.
Ian Lance Taylor (Gerrit)
The alternative is to merge this CL as-is, which means that RawConn will only allow for blocking reads/writes and won't integrate with IOCP. However, this means that read/write deadlines are not respected (as seen by the test hangs in CL 76394)
I'm not especially worried by this special case not handling deadlines correctly, though that would need to be documented.
Aman Gupta (Gerrit)
I'm not especially worried by this special case not handling deadlines correctly, though that would need to be documented.
I think this would be my preference as well. Merge this as-is (after documenting the deadline caveat), and then revisit IOCP+RawConn in the future with a broader API change.
WDYT @mikioh?
Of course if it's too late to merge this for go1.10 anyway, then maybe it doesn't matter either way.
Aman Gupta (Gerrit)
Ping. Can we revisit this now that go1.10 is nearing release?
Brad Fitzpatrick (Gerrit)
R=go1.11
It's definitely too late for Go 1.10, but Go 1.11 opens soon.
Mikio Hara (Gerrit)
WDYT @mikioh?
i think we should provide some useful way for manipulating windows-specific iocp or newer async io mech instead of the half-baked blocking-synchronous-only way, because the half-baked way is completely useless for stateless communication protocols such as udp.
what happens when someone wants to implement modern, udp-based transport protocols such as quic(*) with the half-baked way. at first, they'll see rawconn and have a hope, but then they'll lose the hope because of the lack of functionality; what they really need is an interface that provides a) cooperation with platform-specific io event/multiplex mech and b) knobs for tweaking platform-specific transients, reading/writing byte sequences.
*) they construct a single communication entity (a connection) on top of multiple network-layer addresses and network interfaces; known as multipath support.
Aman Gupta (Gerrit)
i think we should provide some useful way for manipulating windows-specific iocp or newer async io mech instead of the half-baked blocking-synchronous-only way, because the half-baked way is completely useless for stateless communication protocols such as udp.
I agree an API which allows integration with IOCP would be much more preferable. I'm volunteering to work on the implementation if you can advise as to what the API/interface should look like.
In the mean time, I guess I'll pull this patch and the x/net patches into my own forks, because I need ControlMessage support on Windows to be able to implement a multi-interface aware mDNS server (https://github.com/brutella/dnssd/pull/1).
Aman Gupta (Gerrit)
Hi folks, I'm coming back to this after a few months off.
As implemented, this patch only allows users to invoke blocking network i/o from the RawRead/Write callbacks. While this is sufficient for many use-cases (at the expense of extra system threads being blocked on those i/o syscalls), one major issue is that there's no way to timeout/cancel the blocking operation.
A decision was already made that the RawRead/Write interface don't need to respect deadlines, so I'm more concerned with the cancellation aspect rather than timeouts. In a C-program using blocking reads on a thread, another thread could simply closesocket() the handle to effectively cancel the read and unblock the reader thread.
In golang, however, this doesn't work because Close() will wait on any pending read/writes, and closesocket() doesn't get called until (*FD).destroy(). In practice, this means that a go program attempting to gracefully shutdown via Close(), while another goroutine is inside RawRead, will essentially block indefinitely (until/if that RawRead happens to complete because a packet arrived on the network).
On unix based platforms, pollable sockets are created as non-blocking, so the RawRead/Write interface required is very simple. The user-provided callback simply attempts an i/o operation, and returns false on EAGAIN to signal that the runtime should wait on a readable/writable event to try again. Here's how the API in go1.10 is being used in x/net on unix platforms:
data := make([]byte, 65536)
var n int
err := c.RawRead(func (s uintptr) bool {
var errno error
n, _, errno = syscall.Syscall(syscall.SYS_RECVMSG, s, unsafe.Pointer(data))
if errno == syscall.EAGAIN {
return false
}
return true
})
On Windows, sockets are created in blocking-mode, but all operations happen in "overlapped" mode using IOCP (which essentially ignores blocking/non-blocking status). When it comes to RawRead/Write, the user could perform three distinct types of i/o: (a) blocking, (b) non-blocking by setting FIONBIO first, or (c) overlapped using IOCP.
This patch as-is only allows for (a), with something like:
var msg windows.WSAMsg
var qty int
var operr error
err := c.RawRead(func (s uintptr) bool {
operr = windows.WSARecvMsg(syscall.Handle(s), &msg, &qty, nil, nil)
return true
})
To support IOCP (c), the RawRead/Write interface would need to change significantly. For an example of the amount of state and indirection required for an overlapped operation, see (*FD).RecvMsg() [https://github.com/golang/go/commit/e49bc465a#diff-618b3f21201d29fa6d82d50df02dcdbaR949].
As a first pass, one might try to simply add another argument to the callback (as was proposed initially in https://go-review.googlesource.com/c/go/+/37913/1), and use that to pass-through the syscall.Overlapped pointer:
var msg windows.WSAMsg
var qty int
err := c.RawRead(func (s uintptr, o interface{}) bool {
operr := windows.WSARecvMsg(syscall.Handle(s), &msg, &qty, o.(*syscall.Overlapped), nil)
if operr == syscall.ERROR_IO_PENDING {
return false
}
return true
})
However this still not sufficient for IOCP integration, because whenever an I/O operation returns ERROR_IO_PENDING it no longer updates the qty pointer passed to it. So in this case, qty would never be updated with the number of bytes received. Instead, this information is only available when the overlapped operation is reaped later with WSAGetOverlappedResult. Similarly, any flags need to be read from the overlapped result (when using other syscalls like WSARecv).
Further, a boolean return value is also not enough for IOCP, because (for some systems/sockets) the runtime needs to be able to tell when an i/o operation completed immediately so that the IOCP completion message can be reaped and ignored. Returning `true` for both errors and immediate success makes this impossible.
This means that if you were implementing RawRead on top of the IOCP runtime, the signature would need to change, at the very least, as follows:
-RawRead(f func(uintptr) bool) error
+RawRead(f func(uintptr, *syscall.Overlapped) error) (nbytes uint32, flags uint32, err error)
Clearly this is neither portable nor elegant. A step in that direction might look like:
type RawOperation struct {
Operation interface{}
Qty int
Flags int
}
RawRead(*RawOperation, f func(uintptr) error) errorwhich would be used as follows:
var buf windows.WSABuf
var op net.RawOperation
err := c.RawRead(iop, func (s uintptr) error {
var n, flags uint32
operr := windows.WSARecv(syscall.Handle(s), &buf, 1, &n, &flags, op.Operation.(*syscall.Overlapped), nil)
if operr == nil {
op.Qty = int(n)
op.Flags = int(flags)
}
return operr
})
Honestly, that's not much better in either respect. And any change here would be a breaking change (although, as far as I can tell the only consumer of this API is x/net and it was added specifically so x/net could use it).
Long story short, I'm wondering:
1) what is an acceptable iteration of the RawRead/RawWrite functions in the syscall.RawConn interface that would allow for IOCP to be integrated as outlined above?
2) what sort of considerations need to be made as far as not breaking backwards-compatibility of this api? would it be better to add another function like RawReadAsync with the new signature? or are we free to change it at will since it's in the syscall package and only used by x/net
1 comment:
File src/internal/poll/fd_windows.go:
Patch Set #6, Line 905:
if err := fd.pd.waitRead(fd.isFile); err != nil {I copied these from fd_unix.go (as noted in the commit message), but upon further inspection it looks like {prepare,wait}{Read,Write} are not used anywhere else in this file. On unix systems, these functions register the fd with the event loop and then wait for events tied to that fd. But on Windows, calling these in this context makes no sense and does nothing. This is because netpoll_windows.go is entirely IOCP based, which means that it only works with "overlapped" socket *operations* and not the fds themselves.
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.
Aman Gupta (Gerrit)
Since there appears to be no appetite for an API redesign, I've been looking into alternative ways to make the current API workable.
Turns out there's a trick (used by other event loop libraries like libuv) where you can submit a 0-byte read operation to IOCP, and effectively use it as a way to get notified when the socket is readable. I discovered this on https://stackoverflow.com/a/42019668/332798
Using this trick, RawRead can be implemented with the existing API and still give the user the ability to perform non-blocking reads (i.e. reads that don't consume a system thread blocking in the read syscall). To do so, the user would need to either (a) put the socket into non-blocking mode first, or (b) use a socket option like SO_RCVTIMEO, and then return false from their RawRead callback to signal to the runtime that they would like it to wait on readability.
Using this implementation, along with a SO_RCVTIMEO in x/net, I was able to overcome the major issue I was experiencing with this CL in it's current state, as outlined in my previous comment:
In practice, this means that a go program attempting to gracefully shutdown via Close(), while another goroutine is inside RawRead, will essentially block indefinitely (until/if that RawRead happens to complete because a packet arrived on the network).
If this approach is preferred to API changes, I can update this CL accordingly (in the hopes that something could be merged before the go1.11 window closes).
Alex Brainman (Gerrit)
If this approach is preferred to API changes, I can update this CL
accordingly (in the hopes that something could be merged before the
go1.11 window closes).
Speaking for myself only. I would be happy to review whatever solution you have, if it fixes your problem. Perhaps create new CL, so we can decide later. Be warned, I am not network expert - TCP is what I use.
Alex
Aman Gupta (Gerrit)
Aman Gupta uploaded patch set #7 to this change.
net: implement (*syscall.RawConn).Read/Write on Windows
RawRead assumes the callback will perform either (a) a blocking read
and always return true, (b) a blocking read with a SO_RCVTIMEO set
returning false on WSAETIMEDOUT, or (c) a non-blocking read
returning false on WSAEWOULDBLOCK. In the latter two cases, it uses
a 0-byte overlapped read for notifications from the IOCP runtime
when the socket becomes readable before trying again.
RawWrite assumes the callback will perform blocking write and will
always return true, and makes no effort to tie into the runtime loop.
Change-Id: Ib10074e9d502c040294f41a260e561e84208652f
---
M src/internal/poll/fd_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/net/rawconn.go
M src/net/rawconn_windows_test.go
4 files changed, 70 insertions(+), 7 deletions(-)
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.
Aman Gupta (Gerrit)
Speaking for myself only. I would be happy to review whatever solution you have, if it fixes your problem. Perhaps create new CL, so we can decide later.
I've updated this CL with the latest patch. I think this is the best we can do with the current API, and a larger discussion + new CL would be required for an updated API which allows for full IOCP integration.
Alex Brainman (Gerrit)
1 comment:
File src/net/rawconn_windows_test.go:
Patch Set #7, Line 12:
func readRawConn(c syscall.RawConn, b []byte) (int, error) {Which test is this suppose to fix? I run:
```
c:\>go test -v -run=TestRaw net
=== RUN TestRawConnReadWrite
--- SKIP: TestRawConnReadWrite (0.00s)
rawconn_test.go:16: not supported on windows
=== RUN TestRawConnControl
=== RUN TestRawConnControl/TCP
--- PASS: TestRawConnControl (0.00s)
--- PASS: TestRawConnControl/TCP (0.00s)
PASS
Socket statistical information:
(inet4, stream, default): opened=2 connected=0 listened=1 accepted=0 closed=2 op
enfailed=0 connectfailed=1 listenfailed=0 acceptfailed=0 closefailed=0ok net 0.257s
c:\>
```
before and after the change, and your change makes no difference to test output.
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.
Aman Gupta (Gerrit)
Aman Gupta uploaded patch set #8 to this change.
net: implement (*syscall.RawConn).Read/Write on Windows
RawRead assumes the callback will perform either (a) a blocking read
and always return true, (b) a blocking read with a SO_RCVTIMEO set
returning false on WSAETIMEDOUT, or (c) a non-blocking read
returning false on WSAEWOULDBLOCK. In the latter two cases, it uses
a 0-byte overlapped read for notifications from the IOCP runtime
when the socket becomes readable before trying again.
RawWrite assumes the callback will perform blocking write and will
always return true, and makes no effort to tie into the runtime loop.
Change-Id: Ib10074e9d502c040294f41a260e561e84208652f
---
M src/internal/poll/fd_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/net/rawconn.go
M src/net/rawconn_test.go
M src/net/rawconn_windows_test.go
5 files changed, 71 insertions(+), 8 deletions(-)
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.
Aman Gupta (Gerrit)
--- SKIP: TestRawConnReadWrite (0.00s)
rawconn_test.go:16: not supported on windows
Oops, I missed this while fixing the merge conflict in the tests.
Uploaded new patch.
```
go test -v -run=TestRaw net
=== RUN TestRawConn
--- PASS: TestRawConn (0.00s)
=== RUN TestRawConnListener
--- PASS: TestRawConnListener (0.07s)
PASS
Socket statistical information:
(inet4, datagram, default): opened=1 connected=0 listened=0 accepted=0 closed=1 openfailed=0 connectfailed=0 listenfailed=0 acceptfailed=0 closefailed=0
(inet4, stream, default): opened=1 connected=0 listened=1 accepted=0 closed=1 openfailed=0 connectfailed=0 listenfailed=0 acceptfailed=0 closefailed=0
ok net 0.593s
```
Alex Brainman (Gerrit)
I am getting unrelated error:
```
--- FAIL: TestNoRet (0.02s)
asm_test.go:79: exit status 1
mkdir C:\WINDOWS\go-build787602459: Access is denied.
FAIL
FAIL cmd/internal/obj/arm64 0.069s
```
You, probably, need to rebase.
Alex
Patch set 8:Run-TryBot +1
1 comment:
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
Gobot Gobot (Gerrit)
TryBots are happy.
Patch set 8:TryBot-Result +1
Alex Brainman (Gerrit)
I am getting unrelated error:
It seems it only happens to me.
https://github.com/golang/go/issues/24855
Aman, please address my comment, and I will submit.
Thank you.
Alex
Patch set 8:-Run-TryBot
Aman Gupta (Gerrit)
Aman Gupta uploaded patch set #10 to this change.
net: implement (*syscall.RawConn).Read/Write on Windows
RawRead assumes the callback will perform either (a) a blocking read
and always return true, (b) a blocking read with a SO_RCVTIMEO set
returning false on WSAETIMEDOUT, or (c) a non-blocking read
returning false on WSAEWOULDBLOCK. In the latter two cases, it uses
a 0-byte overlapped read for notifications from the IOCP runtime
when the socket becomes readable before trying again.
RawWrite assumes the callback will perform blocking write and will
always return true, and makes no effort to tie into the runtime loop.
Change-Id: Ib10074e9d502c040294f41a260e561e84208652f
---
M src/internal/poll/fd_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/net/rawconn.go
M src/net/rawconn_test.go
M src/net/rawconn_windows_test.go
5 files changed, 71 insertions(+), 8 deletions(-)
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
LGTM once builders are happy.
Thank you.
Alex
Patch set 10:Run-TryBot +1Code-Review +2
Gobot Gobot (Gerrit)
TryBots beginning. Status page: https://farmer.golang.org/try?commit=4d1dd5a1
Gobot Gobot (Gerrit)
Build is still in progress...
This change failed on windows-386-2008:
See https://storage.googleapis.com/go-build-log/4d1dd5a1/windows-386-2008_e6648d1e.log
Consult https://build.golang.org/ to see whether it's a new failure. Other builds still in progress; subsequent failure notices suppressed until final report.
Gobot Gobot (Gerrit)
2 of 17 TryBots failed:
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/4d1dd5a1/windows-386-2008_e6648d1e.log
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/4d1dd5a1/windows-amd64-2016_ac02d57f.log
Consult https://build.golang.org/ to see whether they are new failures.
Patch set 10:TryBot-Result -1
Alex Brainman (Gerrit)
2 of 17 TryBots failed:
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/4d1dd5a1/windows-386-2008_e6648d1e.log
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/4d1dd5a1/windows-amd64-2016_ac02d57f.logConsult https://build.golang.org/ to see whether they are new
failures.
Aman,
Looks like my change https://go-review.googlesource.com/#/c/go/+/106956 that I just submitted affects your CL. Please adjust your CL and I submit. I suspect it is late Friday were you are, so it is OK to leave this for later.
Sorry.
Alex
Patch set 10:-Run-TryBot-Code-Review
Aman Gupta (Gerrit)
Aman Gupta uploaded patch set #11 to this change.
net: implement (*syscall.RawConn).Read/Write on Windows
RawRead assumes the callback will perform either (a) a blocking read
and always return true, (b) a blocking read with a SO_RCVTIMEO set
returning false on WSAETIMEDOUT, or (c) a non-blocking read
returning false on WSAEWOULDBLOCK. In the latter two cases, it uses
a 0-byte overlapped read for notifications from the IOCP runtime
when the socket becomes readable before trying again.
RawWrite assumes the callback will perform blocking write and will
always return true, and makes no effort to tie into the runtime loop.
Change-Id: Ib10074e9d502c040294f41a260e561e84208652f
---
M src/internal/poll/fd_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/net/rawconn.go
M src/net/rawconn_test.go
M src/net/rawconn_windows_test.go
5 files changed, 71 insertions(+), 8 deletions(-)
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Patch set 11:Run-TryBot +1
Gobot Gobot (Gerrit)
TryBots beginning. Status page: https://farmer.golang.org/try?commit=79f1b6c0
Gobot Gobot (Gerrit)
TryBots are happy.
Patch set 11:TryBot-Result +1
Alex Brainman (Gerrit)
Patch set 11:-Run-TryBotCode-Review +2
Alex Brainman (Gerrit)
Alex Brainman merged this change.
net: implement (*syscall.RawConn).Read/Write on Windows
RawRead assumes the callback will perform either (a) a blocking read
and always return true, (b) a blocking read with a SO_RCVTIMEO set
returning false on WSAETIMEDOUT, or (c) a non-blocking read
returning false on WSAEWOULDBLOCK. In the latter two cases, it uses
a 0-byte overlapped read for notifications from the IOCP runtime
when the socket becomes readable before trying again.
RawWrite assumes the callback will perform blocking write and will
always return true, and makes no effort to tie into the runtime loop.
Change-Id: Ib10074e9d502c040294f41a260e561e84208652f
Reviewed-on: https://go-review.googlesource.com/76391
TryBot-Result: Gobot Gobot <go...@golang.org>
Reviewed-by: Alex Brainman <alex.b...@gmail.com>
---
M src/internal/poll/fd_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/net/rawconn.go
M src/net/rawconn_test.go
M src/net/rawconn_windows_test.go
5 files changed, 71 insertions(+), 8 deletions(-)
diff --git a/src/internal/poll/fd_windows.go b/src/internal/poll/fd_windows.go
index 309f029..cd9f88b 100644
--- a/src/internal/poll/fd_windows.go
+++ b/src/internal/poll/fd_windows.go
@@ -913,12 +913,44 @@
// RawRead invokes the user-defined function f for a read operation.
func (fd *FD) RawRead(f func(uintptr) bool) error {
- return errors.New("not implemented")
+ if err := fd.readLock(); err != nil {
+ return err
+ }
+ defer fd.readUnlock()
+ for {
+ if f(uintptr(fd.Sysfd)) {
+ return nil
+ }
+
+ // Use a zero-byte read as a way to get notified when this
+ // socket is readable. h/t https://stackoverflow.com/a/42019668/332798
+ o := &fd.rop
+ o.InitBuf(nil)
+ if !fd.IsStream {
+ o.flags |= windows.MSG_PEEK
+ }
+ _, err := rsrv.ExecIO(o, func(o *operation) error {
+ return syscall.WSARecv(o.fd.Sysfd, &o.buf, 1, &o.qty, &o.flags, &o.o, nil)
+ })
+ if err == windows.WSAEMSGSIZE {
+ // expected with a 0-byte peek, ignore.
+ } else if err != nil {
+ return err
+ }
+ }
}
// RawWrite invokes the user-defined function f for a write operation.
func (fd *FD) RawWrite(f func(uintptr) bool) error {
- return errors.New("not implemented")
+ if err := fd.writeLock(); err != nil {
+ return err
+ }
+ defer fd.writeUnlock()
+ for {
+ if f(uintptr(fd.Sysfd)) {
+ return nil
+ }
+ }
}
func sockaddrToRaw(sa syscall.Sockaddr) (unsafe.Pointer, int32, error) {
diff --git a/src/internal/syscall/windows/syscall_windows.go b/src/internal/syscall/windows/syscall_windows.go
index b531f89..518af26 100644
--- a/src/internal/syscall/windows/syscall_windows.go
+++ b/src/internal/syscall/windows/syscall_windows.go
@@ -122,6 +122,7 @@
WSAEMSGSIZE syscall.Errno = 10040
+ MSG_PEEK = 0x2
MSG_TRUNC = 0x0100
MSG_CTRUNC = 0x0200
diff --git a/src/net/rawconn.go b/src/net/rawconn.go
index 2399c9f..11f01ff 100644
--- a/src/net/rawconn.go
+++ b/src/net/rawconn.go
@@ -9,9 +9,6 @@
"syscall"
)
-// BUG(mikio): On Windows, the Read and Write methods of
-// syscall.RawConn are not implemented.
-
// BUG(mikio): On NaCl and Plan 9, the Control, Read and Write methods
// of syscall.RawConn are not implemented.
diff --git a/src/net/rawconn_test.go b/src/net/rawconn_test.go
index 287282f..ebada13 100644
--- a/src/net/rawconn_test.go
+++ b/src/net/rawconn_test.go
@@ -12,7 +12,7 @@
func TestRawConnReadWrite(t *testing.T) {
switch runtime.GOOS {
- case "nacl", "plan9", "windows":
+ case "nacl", "plan9":
t.Skipf("not supported on %s", runtime.GOOS)
}
diff --git a/src/net/rawconn_windows_test.go b/src/net/rawconn_windows_test.go
index 1b6777b..6df101e 100644
--- a/src/net/rawconn_windows_test.go
+++ b/src/net/rawconn_windows_test.go
@@ -10,11 +10,44 @@
)
func readRawConn(c syscall.RawConn, b []byte) (int, error) {
- return 0, syscall.EWINDOWS
+ var operr error
+ var n int
+ err := c.Read(func(s uintptr) bool {
+ var read uint32
+ var flags uint32
+ var buf syscall.WSABuf
+ buf.Buf = &b[0]
+ buf.Len = uint32(len(b))
+ operr = syscall.WSARecv(syscall.Handle(s), &buf, 1, &read, &flags, nil, nil)
+ n = int(read)
+ return true
+ })
+ if err != nil {
+ return n, err
+ }
+ if operr != nil {
+ return n, operr
+ }
+ return n, nil
}
func writeRawConn(c syscall.RawConn, b []byte) error {
- return syscall.EWINDOWS
+ var operr error
+ err := c.Write(func(s uintptr) bool {
+ var written uint32
+ var buf syscall.WSABuf
+ buf.Buf = &b[0]
+ buf.Len = uint32(len(b))
+ operr = syscall.WSASend(syscall.Handle(s), &buf, 1, &written, 0, nil, nil)
+ return true
+ })
+ if err != nil {
+ return err
+ }
+ if operr != nil {
+ return operr
+ }
+ return nil
}
func controlRawConn(c syscall.RawConn, addr Addr) error {
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.
Mikio Hara (Gerrit)
1 comment:
please revert this. i don't believe that this CL implements syscall.RawConn usefully.
also it's better to file a new issue for syscall.RawConn on windows.
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
1 comment:
please revert this. i don't believe that this CL implements
syscall.RawConn usefully.
This CL fixes TestRawConnReadWrite - that is useful enough for me.
Alex
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
RELNOTE=yes
Aman Gupta (Gerrit)
1 comment:
> please revert this. i don't believe that this CL implements […]
I have this patch deployed across hundreds of Windows 7/8/10 machines implementing multi-homed multicast dns servers. It is quite useful to me.
The original objection to the patchset was that it only allowed for synchronous operations. The version that was merged lets the user choose between blocking, semi-blocking and non-blocking operation as outlined in the commit message.
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.
Mikio Hara (Gerrit)
1 comment:
I have this patch deployed across hundreds of Windows 7/8/10 machines implementing multi-homed multi […]
it's fine each one keeps own opinion, but providing a half-baked api without any caveats is not the good attitude to api users.
it's fine to rewrite the existing caveats, not fine to drop the existing caveats unconditionally.
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
1 comment:
... but providing a half-baked
api without any caveats is not the good attitude to api users.
it's fine to rewrite the existing caveats, not fine to drop the
existing caveats unconditionally.
I don't see what API you are talking about. Windows code was not implemented. Are you worried about Windows broken code is suddenly working for some users? Our own test was disabled on Windows, because it did not work.
If you or anyone else have alternative Windows implementation that works, I am happy to consider it.
Alex
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.
Mikio Hara (Gerrit)
1 comment:
I don't see what API you are talking about.
as far as i can see, there's no caveats for syscall.rawconn on windows that the changelist description describes. is it safe to assume the behavior of syscall.rawconn is consistent across platforms?
Are you worried about Windows broken code is suddenly working for some users?
i don't understand what you are trying to say. i'd like to suggest taking a look at the output of "godoc syscall RawConn."
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
1 comment:
... is it safe to
assume the behavior of syscall.rawconn is consistent across
platforms?
I would not know is syscall.RawConn is consistent across platforms.
i don't understand what you are trying to say.
I am trying to say, that code that make our own test PASS is better than no code.
... i'd like to suggest
taking a look at the output of "godoc syscall RawConn."
I did.
Alex
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.
Mikio Hara (Gerrit)
1 comment:
I am trying to say, that code that make our own test PASS is better than no code.
i'd like to respect your opinion because i'm not a windows user and have no windows stuff. however, as far as i can see, rawconn.write on windows doesn't support the deadline feature provided by runtime-integrated network poller and there are no caveats to it.
again, i'd suggest adding some caveats on syscall.rawconn implementation on windows.
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
1 comment:
again, i'd suggest adding some caveats on syscall.rawconn
implementation on windows.
I do not use syscall.rawconn myself, and I know very little about how it work and why. I will let people who use syscall.rawconn document it.
Alex
To view, visit change 76391. To unsubscribe, or for help writing mail filters, visit settings.