wasi: nonblocking I/O for sockets and pipes on Windows #1579

evacchi · 2023-07-12T19:59:35Z

Further addresses #1500.

Further work to improve the support for nonblocking I/O on Windows. This drops the special-casing for stdin, and instead checks if a handle is a named pipe (which includes sockets and other kinds of pipes); moreover, it further improves _select by introducing a proper wrapper for Winsock's select, which is BSD socket's select, and a compatible implementation of FdSet.

Because we need to distinguish between sockets, pipes, and regular files, the FdSet struct is actually wrapping 3 WinSockFdSets, one for each type; the correct bucket is picket by probing the fd (win Handle) when it's passed to the FdSet method (e.g. FdSet.Set(fd)).

I think this can be further simplified, but I think now it is addressing some of the issues with the current implementation. Further work would be working on poll_oneoff and possibly remove File.PollRead() if the _select() implementations are now good enough for all platforms.

Further work to improve the support to nonblocking I/O on Windows. This drops the special-casing for stdin, and instead checks if a handle is a named pipe (which includes sockets and other kinds of pipes); moreover it further improve _select by introducing a proper wrapper for winsock's select, which is BSD socket's select, and a compatible implementation of FdSet. Signed-off-by: Edoardo Vacchi <[email protected]>

codefromthecrypt

I'm marking this as approve, knowing that we may be digging a hole and some things may not be tenable to support on all platforms. Emulation via concurrency on windows is a high skill low population thing, and we have the skills (the edo), but should still consciously think through things as POSIX emulation get gnarlier and gnarlier.

internal/platform/fdset_windows.go

internal/sysfs/file_windows.go

internal/sysfs/sock_windows.go

codefromthecrypt · 2023-07-18T02:02:54Z

internal/sysfs/select_windows.go

+//
+// "Upon successful completion, the pselect() or select() function shall modify the objects pointed to by the readfds,
+// writefds, and errorfds arguments to indicate which file descriptors are ready for reading, ready for writing,
+// or have an error condition pending, respectively, and shall return the total number of ready descriptors in all the output sets"


As this is emulation with goroutines, it is worthwhile to explain what is in windows, but not possible to use. Also, any downside such as this implies GOMAXPROCS>1 if that's true. This explanation could be in the RATIONALE.md like "Why do we emulate POSIX select on windows?"

💯 on adding a section to RATIONALE. The tl;dr: is that I don't think there is one single API that does everything select does on POSIX, and, because winsock select is blocking under certain conditions we need to spin a goroutine to avoid blocking everything (e.g. I want to check a PIPE vs a socket: I need to peek on the pipe, and select the socket. TBF we could also poll+peek on the socket with select(, ..., timeout:=0) 🤔 )

another important note: TBH even UNIX select is not perfect for our use cases, because it won't honor the clock config of the module; so if you have a "poll 1 sec fd 4", and you implemented it with select, you would end up blocking for literally 1 sec. So if you wanted to do it properly, I think you may have to use some concurrency primitives there as well.

OTOH when this type of I/O is involved I don't even know if it composes with simulated clocks at all.

agree on the tension of real clocks and real files. I found myself in that spaghetti before, thanks for clarifying!

I reworked this to use select with a zero timeout, this simplifies testing and handling quite a bit!

codefromthecrypt · 2023-07-18T02:06:04Z

internal/sysfs/select_windows.go

-		return bytes > 0, err
+		nsocks, errno = winsock_select(rs, ws, es, duration)
+	} else {
+		// Ticker that emits at every pollInterval.


I presume this wasn't extracted to a function because the state "rs, ws, es, duration" or whatever else is needed here, isn't easy to create (and unit test). If it isn't or isn't harder, I might suggest extracting this part and testing it directly vs indirectly via selectAllHandles.

Regardless, if you don't mind doing a coverage pass. It feels like selectAllHandles accepts a context param which would allow reaching some edge cases here (e.g. context already done), but not sure if it is or not.. coverage could tell for sure.

Finally, any area where we rely on goroutines can have some subtle issues and we should be careful. In general stateful concurrent code is the sort of last resort, and I understand this may be the only way out for windows (vs say not supported). Maybe use the hammer test (for the high level function) to make sure the windows calls used below don't have any one-at-a-time things that should be locked. That or say too much.. in any case please consider what may be needed about any concurrency issues here.

I have reworked all this bit and did a coverage pass, adding a few more tests. With the select(timeout:=0) trick there is no longer a user-controlled goroutine, just channels with ticks and timeouts as before.

Signed-off-by: Edoardo Vacchi <[email protected]>

codefromthecrypt

Really good work here. I appreciate the tenacity to eliminate a the goroutine.

I'm not going to subject you to a nit pass. if you feel like anything you can put it in another PR or part of something else, because probably this code will be touched again anyway.

codefromthecrypt · 2023-07-18T23:25:14Z

internal/platform/fdset_windows_test.go

+				}
+				for fd := 0; fd < _FD_SETSIZE; fd++ {
+					isSet := fdSet.IsSet(fd)
+					if fd&0x1 == 0x1 {


require.Equals would be nicer

codefromthecrypt · 2023-07-18T23:25:53Z

internal/platform/fdset_windows.go

+
+// Zero clears the set.
+func (f *WinSockFdSet) Zero() {
+	if f == nil {


unless we use things as nil (later re-assign the pointer) I would leave out the receiver == nil thing for internal types

the receiver can be nil in many places because select accepts fdreads, fdwrites, fdexcepts to be nil. By allowing the receiver to be nil, some error checks can be avoided e.g. f.Count() returning 0 when f == nil, or in this case, zeroing out a nil receiver, which is also useful in select. To be fair, maybe select can be rewritten in a different way so that the nil checks are centralized at the top of the function (maybe replacing nils with empty structs)

I think what made me look was not all functions check nil ;) Anyway it is cool to know at least some of it is helpful!

codefromthecrypt · 2023-07-18T23:27:06Z

internal/sysfs/select_windows.go

+//
+// "Upon successful completion, the pselect() or select() function shall modify the objects pointed to by the readfds,
+// writefds, and errorfds arguments to indicate which file descriptors are ready for reading, ready for writing,
+// or have an error condition pending, respectively, and shall return the total number of ready descriptors in all the output sets"


codefromthecrypt · 2023-07-18T23:27:52Z

internal/sysfs/select_windows_test.go

 	t.Run("peekNamedPipe should report the correct state of incoming data in the pipe", func(t *testing.T) {
 		r, w, err := os.Pipe()
 		require.NoError(t, err)
 		rh := syscall.Handle(r.Fd())
 		wh := syscall.Handle(w.Fd())

-		// Ensure the pipe has data.
+		// Ensure the pipe has no data.
 		n, err := peekNamedPipe(rh)


nit if errno return as the name errno and then use require.EqualErrno as it has better formatting on fail

evacchi · 2023-07-19T12:43:40Z

will do a follow up for the nits; maybe address poll_oneoff too.

evacchi mentioned this pull request Jul 13, 2023

fs: nonblocking writes should use syscall.Write #1581

Merged

evacchi force-pushed the winsock-select branch 2 times, most recently from e920593 to f00cf69 Compare July 17, 2023 14:38

evacchi force-pushed the winsock-select branch from f00cf69 to 0f82d12 Compare July 17, 2023 16:17

evacchi marked this pull request as ready for review July 17, 2023 20:15

evacchi requested review from codefromthecrypt and mathetake as code owners July 17, 2023 20:15

codefromthecrypt approved these changes Jul 18, 2023

View reviewed changes

evacchi added 2 commits July 18, 2023 11:33

formatting

ffa0af6

Signed-off-by: Edoardo Vacchi <[email protected]>

peek sockets instead of selecting

e6bbb48

Signed-off-by: Edoardo Vacchi <[email protected]>

evacchi force-pushed the winsock-select branch from 5ab89da to e6bbb48 Compare July 18, 2023 10:43

evacchi added 4 commits July 18, 2023 18:02

wip

d6795b4

Signed-off-by: Edoardo Vacchi <[email protected]>

wip

4b50041

Signed-off-by: Edoardo Vacchi <[email protected]>

wip

301d324

Signed-off-by: Edoardo Vacchi <[email protected]>

test

0bbeb68

Signed-off-by: Edoardo Vacchi <[email protected]>

evacchi force-pushed the winsock-select branch from d765f06 to 0bbeb68 Compare July 18, 2023 17:05

evacchi added 4 commits July 18, 2023 19:34

more tests

7bbba14

Signed-off-by: Edoardo Vacchi <[email protected]>

coverage

465d214

Signed-off-by: Edoardo Vacchi <[email protected]>

docs

7936d97

Signed-off-by: Edoardo Vacchi <[email protected]>

fix nil

38218db

Signed-off-by: Edoardo Vacchi <[email protected]>

codefromthecrypt approved these changes Jul 18, 2023

View reviewed changes

codefromthecrypt merged commit 1e0c73d into main Jul 18, 2023

codefromthecrypt deleted the winsock-select branch July 18, 2023 23:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wasi: nonblocking I/O for sockets and pipes on Windows #1579

wasi: nonblocking I/O for sockets and pipes on Windows #1579

evacchi commented Jul 12, 2023 •

edited

Loading

codefromthecrypt left a comment

codefromthecrypt Jul 18, 2023

evacchi Jul 18, 2023 •

edited

Loading

codefromthecrypt Jul 18, 2023

evacchi Jul 18, 2023

codefromthecrypt Jul 18, 2023

codefromthecrypt Jul 18, 2023

evacchi Jul 18, 2023

codefromthecrypt left a comment

codefromthecrypt Jul 18, 2023

codefromthecrypt Jul 18, 2023

evacchi Jul 19, 2023

codefromthecrypt Jul 20, 2023

codefromthecrypt Jul 18, 2023

codefromthecrypt Jul 18, 2023

evacchi commented Jul 19, 2023 •

edited

Loading

wasi: nonblocking I/O for sockets and pipes on Windows #1579

wasi: nonblocking I/O for sockets and pipes on Windows #1579

Conversation

evacchi commented Jul 12, 2023 • edited Loading

codefromthecrypt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evacchi Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evacchi commented Jul 19, 2023 • edited Loading

evacchi commented Jul 12, 2023 •

edited

Loading

evacchi Jul 18, 2023 •

edited

Loading

evacchi commented Jul 19, 2023 •

edited

Loading