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

Implement select. #1162

Merged
merged 25 commits into from
Oct 12, 2024
Merged

Implement select. #1162

merged 25 commits into from
Oct 12, 2024

Conversation

sunfishcode
Copy link
Member

Add a select function, defined only on platforms where it doesn't have an FD_SETSIZE limitation.

Add a `select` function, defined only on platforms where it doesn't
have an `FD_SETSIZE` limitation.
In the `waitpid` tests, ensure that the child process has exited, as
dropping `Command` otherwise leaves the process running. This fixes
test hangs on macos.
Comment on lines +21 to +23
readfds: Option<&mut [FdSetElement]>,
writefds: Option<&mut [FdSetElement]>,
exceptfds: Option<&mut [FdSetElement]>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this tested on Win32? It looks like it has a slightly different implementation of FD_SET than Unix does, that isn't implemented in the bitfield way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooh, good spot. I had not yet tested it on Windows. I'll just disable Windows support for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm worried that the current way it's implemented would make Windows support impossible in the future without a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I was thinking of just skipping the Windows tax here until someone shows up with a use case that needs it. If that happens, we could do like a select_ng or whatever until the next semver break.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed my mind here, and decided to re-introduce Windows support, partly because WASI also uses a count+array fd_set like Windows does, and partly because I thought of a way to support this that doesn't radically change the public API.

Windows has a different `FD_SET` representation. Supporting it might be
possible, though the documentation is ambiguous about whether it supports
arbitrary `FD_SETSIZE` values. But even if so, it would require a more
elaborate abstraction, so just drop it for now.
This uses a trick where we still allow users to allocate a
`FdSetElement` array, but we just allocate a `FD_SET` on Windows
out of it.
@sunfishcode sunfishcode merged commit 8493118 into main Oct 12, 2024
43 checks passed
@sunfishcode sunfishcode deleted the sunfishcode/select branch October 12, 2024 00:14
sunfishcode added a commit that referenced this pull request Oct 17, 2024
* Implement `select`.

Add a `select` function, defined only on platforms where it doesn't
have an `FD_SETSIZE` limitation.

* Fix test hangs on macos.

In the `waitpid` tests, ensure that the child process has exited, as
dropping `Command` otherwise leaves the process running. This fixes
test hangs on macos.

* Wait for the child process after signaling it.

* Fix the vector sizes in the test.

* Add `fd_set` and other convenience functions.

* Switch to a safe API.

* Support `select` on Linux and Windows too.

This uses a trick where we still allow users to allocate a
`FdSetElement` array, but we just allocate a `FD_SET` on Windows
out of it.

And make `select` unsafe due to I/O safety.

* Fix qemu to implment arbitrary-sized fd sets for `select`.

* Support WASI.

* Ignore "unstable name collisions" warnings for now.
sunfishcode added a commit that referenced this pull request Oct 17, 2024
* Implement `select`.

Add a `select` function, defined only on platforms where it doesn't
have an `FD_SETSIZE` limitation.

* Fix test hangs on macos.

In the `waitpid` tests, ensure that the child process has exited, as
dropping `Command` otherwise leaves the process running. This fixes
test hangs on macos.

* Wait for the child process after signaling it.

* Fix the vector sizes in the test.

* Add `fd_set` and other convenience functions.

* Switch to a safe API.

* Support `select` on Linux and Windows too.

This uses a trick where we still allow users to allocate a
`FdSetElement` array, but we just allocate a `FD_SET` on Windows
out of it.

And make `select` unsafe due to I/O safety.

* Fix qemu to implment arbitrary-sized fd sets for `select`.

* Support WASI.

* Ignore "unstable name collisions" warnings for now.
sunfishcode added a commit that referenced this pull request Oct 20, 2024
* Implement `select`.

Add a `select` function, defined only on platforms where it doesn't
have an `FD_SETSIZE` limitation.

* Fix test hangs on macos.

In the `waitpid` tests, ensure that the child process has exited, as
dropping `Command` otherwise leaves the process running. This fixes
test hangs on macos.

* Wait for the child process after signaling it.

* Fix the vector sizes in the test.

* Add `fd_set` and other convenience functions.

* Switch to a safe API.

* Support `select` on Linux and Windows too.

This uses a trick where we still allow users to allocate a
`FdSetElement` array, but we just allocate a `FD_SET` on Windows
out of it.

And make `select` unsafe due to I/O safety.

* Fix qemu to implment arbitrary-sized fd sets for `select`.

* Support WASI.

* Ignore "unstable name collisions" warnings for now.
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.

2 participants