Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement vecnet.ReadFrom on Windows #54

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

djdv
Copy link
Collaborator

@djdv djdv commented May 25, 2022

This implementation is mostly copied from the Linux version, with types and methods being adjusted for Windows.

I wrote a test which covers this and passes, but I'm not sure how correct it is.
The main concern is if the calls to WriteMsgUDP would ever block. If they do, this could just be wrapped in a goroutine.
Maybe that happens on larger messages? Maybe the test should send 9P-messages instead of arbitrary []bytes?

Outside of this though, I'm not sure how it compares to the existing io.Read fallback in vecnet.ReadFrom(reader).
Both in terms of performance or correctness.

I'll leave this as a draft until it's tested a little better with implementations that use the library across Windows<->Linux machines.
As long as they appear to comply and the performance isn't worse, it should be okay I guess?

Edit: I'm getting hangs during Attach sometimes. Maybe it's the opposite of above, small messages don't ever get flushed from the buffers?
Not sure but not good enough to be considered stable. (*Could always have been a problem in my server implementation too.)

func recvmsg(bufs Buffers, rc syscall.RawConn) (int, error) {
var (
bytesReceived uint32
WSABufs = buildWSABufs(bufs, make([]windows.WSABuf, 0, 2))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving a note that this magic 2 for the capacity is copied from vecnet_linux.go which does the same.
Not sure the original rationale for using 2 other than it being a reasonable alloc default (as opposed to 0 since we immediately append to it within buildWSABufs)

If there's a better value (typical average bufcount?), it could be used here. But this is probably fine as-is.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually probably stole the 2 from gVisor and didn't think about it further.

}
}

func newLocalListener(t *testing.T) (*net.UDPConn, *net.UDPAddr) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. UDP was chosen here mainly for compat and simplicity, but could easily be changed to use Unix domain sockets instead.
    I could be wrong, but I don't think the message passing semantics differ between them (especially when local).
    In addition, (modern) Unices and Windows typically support UDS, so instead of letting the system choose a UDP port for us, we could create (and cleanup) socket files during the test. But I don't think we have to make this change, so I won't unless requested.

  2. It should be noted that pkg net (as of 1.19) implies all the MsgX methods are not implemented on Plan9 regardless of transport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants