Support VxWorks, Fuchsia and other Unix systems by using poll #26

Kestrer · 2020-12-17T19:04:05Z

Closes #20.

Notes:

I had to add the extra_traits feature to libc to #[derive(Debug)] on the polling implementation. This can be done manually to avoid activating the feature, if that's something you care about.
I had to #[derive(Clone, Copy)] on Event. It seems to be fine as Event's interface is already fully exposed.
I have tested this on Linux by disabling epoll and I have checked it compiles on Fuchsia (VxWorks and Haiku aren't supported by Rustup).

ghost · 2020-12-17T19:38:31Z

Some quick questions before I do a deeper dive into the code...

Suppose a thread is blocked on wait(). Another thread then calls add() and the added FD is immediately ready.

Is it ensured that wait() doesn't hold a mutex lock that blocks add() forever?
Is it ensured that add() wakes up the caller of wait() even though the added FD is not in its poll set?

Kestrer · 2020-12-17T20:29:32Z

Thanks for pointing that out, I didn't consider those cases - I'll convert this to a draft.

Kestrer · 2020-12-18T15:09:16Z

Ok, I think I've handled those cases now. Any call to add, modify or delete will interrupt the thread that's calling poll and send it into a condvar loop which it will exit if the user calls .notify() or all the operations complete.

src/poll.rs

fogti · 2021-05-25T18:58:03Z

src/poll.rs

+    /// The list of `pollfds` taken by poll.
+    ///
+    /// The first file descriptor is always present and is used to notify the poller. It is also
+    /// stored in `notify_read`.
+    ///
+    /// If the fd stored in here is `REMOVE_FD`, it should be removed.
+    poll_fds: Vec<libc::pollfd>,
+    /// The map of each file descriptor to data associated with it. This does not include the file
+    /// descriptors `notify_read` or `notify_write`.
+    fd_data: HashMap<RawFd, FdData>,


This seems to just implement an separately-ordered, hashmap. Why don't we pull in something like indexmap for this, or do we require some feature it doesn't have? (if true, which?)

I'm averse to adding a dependency for this implementation, mostly because it would require a really long cfg in the Cargo.toml 😄. However, this comment did make me realize that I can make the implementation more efficient and simpler by using swap_remove instead of marking fds as removed and later removing them.

fogti · 2021-05-26T12:38:07Z

We use

[target."cfg(unix)".dependencies]
libc = { version = "0.2.77", features = ["extra_traits"] }

but

    } else if #[cfg(any(
        target_os = "vxworks",
        target_os = "fuchsia",
        unix,
    ))] {
        mod poll;
        use poll as sys;

and

// std::os::unix doesn't exist on Fuchsia
use libc::c_int as RawFd;

This is a mismatch between cfg-targets and should be fixed.
btw. if fixed, this would mean the justification in "I'm averse to adding a dependency for this implementation, mostly because it would require a really long cfg in the Cargo.toml" would no longer apply....

Kestrer · 2021-05-26T13:00:27Z

Ah right, the reason Fuchsia was compiling before is that it hasn't actually been changed to a target_family of None yet (it's still Unix). But now I've future-proofed it everywhere.

if fixed, this would mean the justification in "I'm averse to adding a dependency for this implementation, mostly because it would require a really long cfg in the Cargo.toml" would no longer apply....

It still applies, because the actual cfg required there would be cfg(any(target_os = "fuchsia", target_os = "vxworks", all(unix, not(any(target_os = "linux", target_os = "android", target_os = "illumos", target_os = "solaris", target_os = "macos", target_os = "ios", target_os = "freebsd", target_os = "netbsd", target_os = "openbsd", target_os = "dragonfly"))))).

fogti · 2021-05-26T13:03:50Z

Ah ok, didn't notice that libc is used in really many places...

link2xt · 2021-07-28T19:04:10Z

I have tested it on Haiku, works for me. Rustup indeed does not support Haiku, but rust can be installed with pkgman install rust_bin.

taiki-e · 2021-08-03T11:10:02Z

I haven't reviewed this PR yet, but in mio, (IIUC) there was a discussion on poll's problems in the past: tokio-rs/mio#1472 (comment)
How does this PR deal with such problems?

link2xt · 2021-08-03T13:26:11Z

I haven't reviewed this PR yet, but in mio, (IIUC) there was a discussion on poll's problems in the past: tokio-rs/mio#1472 (comment)
How does this PR deal with such problems?

By storing all file descriptors inside a Poller, it's the first field of type Mutex<Fds>. Looks like polling does not have problems supporting poll.

taiki-e

This looks good to me. Thanks for working on this!
(I'm not familiar with the platforms mentioned here, but I have tweaked this patch a bit and tested polling, async-io, and async-net on macOS.)

Could you update "supported platforms" docs on readme?

Could you add Fuchsia to cross workflow on CI? Probably something like:

- name: Fuchsia
  if: startsWith(matrix.os, 'ubuntu')
  run: cross build --target x86_64-fuchsia

src/poll.rs

Cargo.toml

taiki-e · 2021-08-28T09:09:29Z

Also, would it be possible to add tests for the cases mentioned in #26 (comment) by Stjepan? I don't think tests for such a case currently exist.

Suppose a thread is blocked on wait(). Another thread then calls add() and the added FD is immediately ready.

Is it ensured that wait() doesn't hold a mutex lock that blocks add() forever?

Is it ensured that add() wakes up the caller of wait() even though the added FD is not in its poll set?

Kestrer · 2021-09-04T18:25:42Z

OK, I have done everything suggested 😊

Hopefully CI will pass...

taiki-e

LGTM, Thanks @Kestrer!

taiki-e · 2021-11-10T10:36:58Z

Published in v2.2.0.

Kestrer added 2 commits December 17, 2020 18:56

Use poll on VxWorks, Fuchsia and other Unix systems

2fc0831

Don't set CURRENT_WEEK in CI

0f2f6ed

Kestrer marked this pull request as draft December 17, 2020 20:29

Kestrer added 3 commits December 18, 2020 14:18

Make poll's poller modifications interrupt wait

e0789a8

Add logging to poll backend

dd15b4c

Have notifications break poll's operation loop

20c1e19

Kestrer closed this Dec 18, 2020

Kestrer reopened this Dec 18, 2020

Kestrer marked this pull request as ready for review December 18, 2020 17:56

Merge branch 'master'

a04a265

fogti reviewed May 25, 2021

View reviewed changes

src/poll.rs Outdated Show resolved Hide resolved

fogti reviewed May 25, 2021

View reviewed changes

Kestrer added 2 commits May 26, 2021 10:25

swap_remove deleted file descriptors in poll backend

e0c0032

Prevent timeout overflow for poll and wepoll

c174c9b

Depend on libc on Fuchsia and VxWorks

6d2ccc4

ivmarkov mentioned this pull request Aug 17, 2021

error when compiling mio ivmarkov/rust-esp32-std-demo#9

Closed

taiki-e requested changes Aug 28, 2021

View reviewed changes

src/poll.rs Outdated Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

Kestrer added 3 commits September 4, 2021 17:47

Cast to nfds_t instead of u64 in poll

9c257cc

Remove extra_traits libc feature

ee13911

Document poll support in README

1930801

Kestrer added 2 commits September 4, 2021 18:01

Add Fuchsia to cross workflow on CI

bbce346

Add concurrent modification tests

597b6ae

Add Fuchsia target before crosscompiling in CI

e902924

taiki-e approved these changes Sep 11, 2021

View reviewed changes

taiki-e merged commit 4ac15ff into smol-rs:master Sep 11, 2021

taiki-e mentioned this pull request Dec 29, 2022

Test the backend that uses poll on CI #63

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support VxWorks, Fuchsia and other Unix systems by using poll #26

Support VxWorks, Fuchsia and other Unix systems by using poll #26

Kestrer commented Dec 17, 2020 •

edited

Loading

ghost commented Dec 17, 2020

Kestrer commented Dec 17, 2020

Kestrer commented Dec 18, 2020

fogti May 25, 2021

Kestrer May 26, 2021

fogti commented May 26, 2021

Kestrer commented May 26, 2021 •

edited

Loading

fogti commented May 26, 2021

link2xt commented Jul 28, 2021 •

edited

Loading

taiki-e commented Aug 3, 2021 •

edited

Loading

link2xt commented Aug 3, 2021

taiki-e left a comment

taiki-e commented Aug 28, 2021

Kestrer commented Sep 4, 2021

taiki-e left a comment

taiki-e commented Nov 10, 2021

Support VxWorks, Fuchsia and other Unix systems by using poll #26

Support VxWorks, Fuchsia and other Unix systems by using poll #26

Conversation

Kestrer commented Dec 17, 2020 • edited Loading

ghost commented Dec 17, 2020

Kestrer commented Dec 17, 2020

Kestrer commented Dec 18, 2020

fogti May 25, 2021

Choose a reason for hiding this comment

Kestrer May 26, 2021

Choose a reason for hiding this comment

fogti commented May 26, 2021

Kestrer commented May 26, 2021 • edited Loading

fogti commented May 26, 2021

link2xt commented Jul 28, 2021 • edited Loading

taiki-e commented Aug 3, 2021 • edited Loading

link2xt commented Aug 3, 2021

taiki-e left a comment

Choose a reason for hiding this comment

taiki-e commented Aug 28, 2021

Kestrer commented Sep 4, 2021

taiki-e left a comment

Choose a reason for hiding this comment

taiki-e commented Nov 10, 2021

Kestrer commented Dec 17, 2020 •

edited

Loading

Kestrer commented May 26, 2021 •

edited

Loading

link2xt commented Jul 28, 2021 •

edited

Loading

taiki-e commented Aug 3, 2021 •

edited

Loading