Skip to content

feat(iocp): prefer async connect #158

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nikneym
Copy link
Contributor

@nikneym nikneym commented Apr 14, 2025

Closes #151. API is not changed; all tests are passing as before.

@nikneym
Copy link
Contributor Author

nikneym commented May 8, 2025

cc @Corendos I'd be glad if you could review this one too 👀

Copy link
Contributor

@Corendos Corendos left a comment

Choose a reason for hiding this comment

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

Left some nits, but once resolved, it's ready to merge ! 😁

On a side note, Windows keep on delivering in weirdness sometimes. What is this required dynamic loading of ConnectEx 😂

else => .{ .result = .{ .connect = windows.unexpectedWSAError(err) } },
const as_socket = asSocket(v.socket);
// Associate our socket with loop's completion port.
self.associate_fd(v.socket) catch unreachable;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.associate_fd(v.socket) catch unreachable;
self.associate_fd(completion.handle().?) catch unreachable;

Don't forget to update Completion.handle as well though

Comment on lines +554 to +563
// NOTE: This can be declared in somewhere else; it all happens in comptime though so no issue putting it here.
const LPFN_CONNECTEX = *const fn (
Socket: windows.ws2_32.SOCKET,
SockAddr: *const windows.ws2_32.sockaddr,
SockLen: posix.socklen_t,
SendBuf: ?*const anyopaque,
SendBufLen: windows.DWORD,
BytesSent: *windows.DWORD,
Overlapped: *windows.OVERLAPPED,
) callconv(.winapi) windows.BOOL;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be moved to src/windows.zig

// The following is necessary for various functions (shutdown, getsockopt, setsockopt, getsockname, getpeername) to work.
// https://learn.microsoft.com/en-us/windows/win32/api/mswsock/nc-mswsock-lpfn_connectex#remarks
// https://stackoverflow.com/questions/13598530/connectex-requires-the-socket-to-be-initially-bound-but-to-what
_ = windows.ws2_32.setsockopt(asSocket(v.socket), windows.ws2_32.SOL.SOCKET, windows.ws2_32.SO.UPDATE_CONNECT_CONTEXT, null, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_ = windows.ws2_32.setsockopt(asSocket(v.socket), windows.ws2_32.SOL.SOCKET, windows.ws2_32.SO.UPDATE_CONNECT_CONTEXT, null, 0);
_ = windows.ws2_32.setsockopt(as_socket, windows.ws2_32.SOL.SOCKET, windows.ws2_32.SO.UPDATE_CONNECT_CONTEXT, null, 0);

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.

win: prefer LPFN_CONNECTEX for asynchronous connect calls
2 participants