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

wasi: sockets, use WSAPoll and GetOverlappedResult on Windows #1903

Merged

Conversation

gaukas
Copy link
Contributor

@gaukas gaukas commented Jan 7, 2024

Current readSocket function for WASI sockets is implemented incorrectly on Windows and WASI polling an async socket for a long time will eventually get ERROR_WORKING_SET_QUOTA (1453):

Insufficient quota to complete the requested service.

A call to GetOverlappedResult is expected after each ERROR_IO_PENDING returned by ReadFile and that is what's missing in the current implementation. Besides that, ReadFile would create a background worker to keep on trying reading from the handle into the buf, which could result in data loss if we don't keep tracking of it. So we add a call to WSAPoll before calling ReadFile to observe how many bytes are ready to be read.

In this pull request, we modify how readSocket in internal/sysfs/file_windows.go is implemented by adding the following changes:

  • Call WSAPoll before calling ReadFile, abort if no bytes are available no fd is ready to be read.
  • Call GetOverlappedResult if ReadFile returns ERROR_IO_PENDING.
  • Limit the read buffer's size according to how many bytes are ready to be read (see review comments)
  • Cancel the ongoing overlapped read if it cannot be completed immediately.

syscall.Read on a non-blocking socket will error with incorrect parameters on Windows.

Signed-off-by: Gaukas Wang <[email protected]>
@gaukas gaukas force-pushed the sock-windows-async-fix branch from c3d9b7d to c653347 Compare January 7, 2024 22:33
internal/sysfs/file_windows.go Outdated Show resolved Hide resolved
Copy link
Contributor

@evacchi evacchi left a comment

Choose a reason for hiding this comment

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

I would add a test case.

You can modify

func TestTcpConnFile_Read(t *testing.T) {
to ensure that it also checks the conditions that caused this PR in the first place (You might need to modify the WASI test case)

You can also add a unit test to readSocket (add a test to file_test.go, there is none currently.)

You can do either or both. Thanks!

gaukas added 3 commits January 8, 2024 15:32
Cancel the async read started by us if it cannot be completed immediately.

Signed-off-by: Gaukas Wang <[email protected]>
Document the bug which could not be reproduced in a test case. Remove the fix applied.

Signed-off-by: Gaukas Wang <[email protected]>
@gaukas gaukas requested a review from evacchi January 8, 2024 23:39
Copy link
Contributor

@evacchi evacchi left a comment

Choose a reason for hiding this comment

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

clean up the commented out bits, if the tests pass then it should be good to go!

@gaukas gaukas requested a review from evacchi January 9, 2024 18:44
@mathetake mathetake merged commit 5872ab6 into tetratelabs:main Jan 9, 2024
44 checks passed
@gaukas gaukas deleted the sock-windows-async-fix branch January 9, 2024 22:04
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.

3 participants