x/sys/windows: fix Accept implementation

[x1unix]

Currently windows.Accept (like syscall.Accept for GOOS=windows) contains a stub function which does nothing.

Looks like this function was introduced to mimic unix.Sockaddr.Accept method but it's just a useless stub.

I propose to fix it's signature and re-implement it as winsock's accept function.

In the end it will look something like this:

package windows

func Accept(fd windows.Handle, sockaddr, sockaddrLen uintptr)

Winsock accept function signature (see MSDN)

SOCKET WSAAPI accept(
  [in]      SOCKET   s,
  [out]     sockaddr *addr,
  [in, out] int      *addrlen
);

Instead of passing sockaddr raw pointer and it's size it's possible to also use Sockaddr interface and the same approach like in windows.Bind but it will limit method's potential use cases (it can't be used for custom sockets). See #54209.

[ianlancetaylor]

Windows programs mostly use windows.AcceptEx. If Accept can work I don't see a problem implementing it. But the only reason to implement it in x/sys/windows would be to match x/sys/unix, meaning that the function should be

func Accept(fd Handle) (nfd Handle, sa Sockaddr, err error)

[x1unix]

@ianlancetaylor the problem is that in x/sys/unix, Accept is a method of Sockaddr interface but in windows package it's a dedicated function which just accepts some kind of handle (which makes it useless).

Winsock supports a classic accept function and accepts socket, sockaddr pointer and sockaddr struct length.

I would recommend to implement Sockaddr interface in the same way as it's done in x/sys/unix but to make it open for custom implementation so it can be reused for custom sockets.

[rsc]

For Unix, Accept shows up next to Sockaddr in pkg.go.dev because it is a constructor, not a method.

The Accept signature in x/sys/windows can't be changed, but it would be fine to implement.
If the signature is not changing and the behavior is changing from a stub to an actual implementation, that sounds perfectly fine and doesn't need more discussion.

Do I have that right?

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[x1unix]

 @rsc I'm not really sure that it's possible to implement Accept method for windows with only 1 argument, because in winsocks signature is the same as for regular POSIX accept method.

That means that accept requires also sockaddr pointer and a size:

SOCKET WSAAPI accept(
  [in]      SOCKET   s,
  [out]     sockaddr *addr,
  [in, out] int      *addrlen
);

There also an AcceptEx function but it's much more complicated and non POSIX-compatible.

Do you know why it was initially designed with only 1 argument? And how it was supposed to be used?

Thank you.

[ianlancetaylor]

The Windows accept function is the same as the Unix accept function. But the addr and addrlen parameters are just output parameters. There is no need to pass them in. So the Go version just passes down the address of local variables, and returns those variables. See, for example, https://go.googlesource.com/sys/+/refs/heads/master/unix/syscall_bsd.go#272.

[x1unix]

@ianlancetaylor thank you for clarification :)

[PumpkinSeed]

The Windows accept function is the same as the Unix accept function. But the addr and addrlen parameters are just output parameters. There is no need to pass them in. So the Go version just passes down the address of local variables, and returns those variables. See, for example, https://go.googlesource.com/sys/+/refs/heads/master/unix/syscall_bsd.go#272.

One quick clarification, the addrlen is an [in, out] parameter, which means that it receives a value and based on that value it will refresh the value on the same memory address. I don't have experience with Accept specifically, but in some cases it's mandatory to pass the value into the syscall, and used the refreshed one. One specific example: WSALookupServiceNext also accepts a size, which value is 120 in the first syscall (the length of WSAQUERYSET), and updates to 1024. The next syscall supposed to used the same memory address and the refreshed value.

This doesn't mean that you need to update the function signature, but something to keep in mind.

[x1unix]

@rsc can I contribute somehow to the implementation if this proposal will be accepted?

[ianlancetaylor]

@x1unix Sure. Thanks.

[rsc]

It sounds like everyone agrees this is about implementing a stub function, keeping the API exactly the same. Assuming that's true, there's no need for a proposal.

[rsc]

Removing from the proposal process now that the issue is simply to implement the existing stub API with no API signature changes or new APIs.

[rsc]

Removed from the proposal process.
This was determined not to be a “significant change to the language, libraries, or tools”
or otherwise of significant importance or interest to the broader Go community.
— rsc for the proposal review group