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

sysfs: implements PollRead with poll instead of select #1596

Merged
merged 3 commits into from
Jul 28, 2023
Merged

Conversation

evacchi
Copy link
Contributor

@evacchi evacchi commented Jul 26, 2023

In all usages, it looks like poll should be preferred to select: the API is simpler and easier to emulate on other platforms; above all, it does not use FdSets and won't change pointers in-place.

We remove all usages of select and FdSet and instead expose and use poll and []PollFd, which is comparatively much more straightforward. We also rewrite the select emulation on Windows, and instead emulate poll, based on the same principle: we use WSAPoll for sockets, PeekNamedPipes for pipes, and report all regular files as "ready".

This in turn should eventually make it possible to remove File.PollRead or at the very least simplify further poll_oneoff.

@evacchi evacchi requested a review from achille-roussel July 26, 2023 17:40
@evacchi evacchi marked this pull request as ready for review July 27, 2023 15:24
internal/sysfs/poll.go Outdated Show resolved Hide resolved
internal/sysfs/poll_unsupported.go Show resolved Hide resolved
Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

I messed up the other API, signature wise (duration not int64) and will fix in another PR.

Meanwhile, this looks good! I was verbose about style stuff so that we can try to be coherent with other style by default. This helps the project grow and feel to users as if it was written by one person, even if that's never the case.

Cheers.

internal/sysfs/poll.go Outdated Show resolved Hide resolved
internal/sysfs/poll.go Outdated Show resolved Hide resolved
codefromthecrypt pushed a commit that referenced this pull request Jul 28, 2023
This changes `PollRead` to accept int32 timeoutMillis instead of using a
pointer. I made a mistake earlier on this API, which wasn't being direct
about units and also using nil where it isn't used that way at the
syscall layer.

See #1596

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt
Copy link
Contributor

I opened #1597 to rebase the existing implementation, which will drift this. If it merges today, I will rebase your PR!

Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
@codefromthecrypt codefromthecrypt changed the title fs: introduce sysfs.poll to replace _select sysfs: implements PollRead with poll instead of select Jul 28, 2023
Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

OK I reworked this to pare down the change around implementing PollRead with poll instead of select, which is the main part. Main thing I think waiting here is to revisit the RATIONALE which is about select instead of poll. More work can happen in future PRs, but be wary of platforms that don't support syscalls such as plan9. More on that below!

A future change can implement a multi-poll, but that wouldn't seem to make sense on a file because it has only one file descriptor. In fact I would probably suggest not doing this until forced to, because there are a lot of edge cases we have to test. Even here we have logic to handle many types of inputs, but the only one tested is a single fd on read. In other words, unsolicited 2p is to not let the impl grow too far in front of reqiuirements and lazy grow the impl only when tests and ABI moves to require them.

@codefromthecrypt
Copy link
Contributor

build break seems unrelated

 /usr/bin/sh: line 1: osslsigncode: command not found
mingw32-make: *** [makefile:302: build/wazero_windows_amd64/wazero.exe.signed] Error 127

@evacchi
Copy link
Contributor Author

evacchi commented Jul 28, 2023

further work would be generalize the use of poll in poll_oneoff (right now it is only used for stdin). I am now updating RATIONALE.md

Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

RATIONALE LGTM

Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
@evacchi evacchi merged commit 023a383 into main Jul 28, 2023
@evacchi evacchi deleted the syscall-poll branch July 28, 2023 08:49
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.

4 participants