Calculate `nfds` parameter for `select` #701

jonas-schievink · 2017-07-20T22:48:24Z

Doing this behind the scenes makes the API less error-prone and easier
to use. It should also fix my issue in #679 (comment)

jonas-schievink · 2017-07-20T22:49:36Z

src/sys/select.rs

+    /// Finds the largest file descriptor in the set.
+    ///
+    /// Returns `None` if the set is empty.
+    pub fn largest(&self) -> Option<RawFd> {


I had to make this function public in order to add a test. For some reason putting a #[test] function in this module didn't run the test.

Hmm, it definitely should. Can you post the code with it written as a private method like you'd expect it to work?

Ah, nevermind, it does work. I probably typo'd #[cfg(test)] before. But since the other tests for this type are inside test_select.rs, I'm going to leave it this way unless you object.

I'd rather have largest() be private. And in general unit tests should be in the same file as the functions themselves, it's only the larger integration tests that should be in /tests. Let's more the tests into this file at the bottom in a test module.

Susurrus · 2017-07-20T22:51:41Z

test/sys/test_select.rs

@@ -31,6 +31,20 @@ fn test_fdset() {
    }
 }

+#[test]


This isn't needed here.

It is, otherwise rustc never calls this method (I just checked by putting a panic! inside). Maybe you're thinking of some other test file without harness?

Sorry, you're right!

jonas-schievink · 2017-07-20T23:17:10Z

Note that this is technically not zero-cost, but it is pretty close since the array is small. selects performance isn't that great even if it were, though :)

jonas-schievink · 2017-07-20T23:30:35Z

src/sys/select.rs

+        for (i, &block) in self.bits.iter().enumerate().rev() {
+            if block != 0 {
+                // highest bit is located at `n.trailing_zeros()`
+                return Some((i * BITS + block.trailing_zeros() as usize) as RawFd);


Ah, there's a bug in here.

asomers

I like makeing FdSet::largest a public method, but I have misgivings about making it mandatory. Even though select is not fast compared to kqueue or the like, and even though FD_SETSIZE is not super big, changing an O(1) algorithm into an O(n) algorithm is still not good. Better to give the consumer the option of using largest or simply calculating nfds himself.

The simplest solution to the test_select failure is just to pass max(r1, r2) + 1 as `nfds.

Susurrus · 2017-07-21T03:54:46Z

src/sys/select.rs

@@ -68,11 +87,18 @@ mod ffi {
    }
 }

-pub fn select(nfds: c_int,
-              readfds: Option<&mut FdSet>,
+pub fn select(readfds: Option<&mut FdSet>,


If you're feeling particularly ambitious, this would be a good time to change the definiton of select to something like pub fn select<T: Into<Option<&mut FdSet>>>(readfds: T ... such that all the Option types have those kinds of signatures. Then you can use .into() within the function and the users of the functions don't need to wrap arguments in Some(). It's really ergonomic!

Susurrus · 2017-07-21T03:55:34Z

src/sys/select.rs

              writefds: Option<&mut FdSet>,
              errorfds: Option<&mut FdSet>,
              timeout: Option<&mut TimeVal>) -> Result<c_int> {
+    // calculate nfds parameter, the largest fd we want to select on
+    let nfds = readfds.iter()
+        .chain(writefds.iter())


Can you indent these to align the "."s vertically? Much more readable then.

Will do, but please consider moving to the proposed RFC style. Most editors make it surprisingly hard to indent code like this.

jonas-schievink · 2017-07-21T08:46:47Z

@asomers It's not O(n), though. The loop runs a maximum of FD_SETSIZE / BITS iterations, which is a constant, so it's still O(1). The array only contains 16 or 32 integers for the defined platforms, which should be completely unnoticeable (although I haven't tested). Performance of the calculation even improves with more fds in the set (or higher-numbered ones).

So, what should I do about largest? I can make it pub(crate) at best, not completely private.

jonas-schievink · 2017-07-21T11:18:38Z

Okay, I just tried to re-enable the test on mips and powerpc, but it looks like it still fails.

Susurrus · 2017-07-30T22:49:00Z

src/sys/select.rs

+    use unistd::{write, pipe};
+
+    #[test]
+    fn test_fdset() {


This should probably be broken into a few smaller tests that make it more clear as to what's being tested. Like break up the adding an Fd into a test, then test adding & removing, then test clear in a separate test, etc. It makes debugging failures much easier if tests are grouped into a small amount of related tests. This has all the tests in one function.

asomers · 2017-07-30T23:19:01Z

I still disagree with automatically calculating nfds. Rust is all about zero-cost abstractions, and nix is no different. But I like the idea of exposing largest to the user.

jonas-schievink · 2017-07-30T23:22:42Z

Okay, I'll make that optional

asomers · 2017-07-31T01:12:16Z

Interesting making nfds optional. I guess that gives the best of both worlds. Everything here looks good to me, though it still needs to be squashed. Also, please add a doccomment for select, and add an entry to CHANGELOG documenting the API change.

Susurrus · 2017-07-31T14:26:29Z

src/sys/select.rs

@@ -53,6 +53,25 @@ impl FdSet {
            *bits = 0
        }
    }
+
+    /// Finds the largest file descriptor in the set.


I don't think largest is an appropriate term here, as RawFds don't have a size. They have a numbering, however, so highest might be appropriate here. Also, since this basically exists exclusively for use with select(), please add a simple example showing how you might use it here.

jonas-schievink · 2017-08-02T15:25:45Z

@Susurrus @asomers Anything left to do here?

Susurrus · 2017-08-02T15:30:02Z

You need to rebase this and move your entries into the CHANGELOG into the new Unreleased section since this no longer applies to the 0.9 release.

I'm not a fan of leaving the nfds computation up to the user and would prefer to have it handled exclusively by nix given that it is O(1) time. We have non-zero-cost abstractions around other APIs (like termios) that I don't consider a problem because the ergonomics/API-safety win outweighs the performance. Users always have an easy escape-hatch from nix, and that's to use libc directly so I'm not too worried about that. But @asomers seems to not like that idea and I'll defer to him on this.

asomers · 2017-08-02T15:31:02Z

LGTM. Thanks for all the hard work.

bors r+

asomers · 2017-08-02T15:34:00Z

I'm satisfied, but for the CHANGELOG issue.

asomers · 2017-08-02T15:34:18Z

bors, wait for the changelog to be fixed.

bors r-

jonas-schievink · 2017-08-02T15:45:54Z

Ah, good catch about the changelog, didn't notice that I broke it during rebase. Should be fixed now.

Susurrus

One thing that isn't addressed here is how to use the select() function in the various ways that are now supported, both in documentation and in test code. There are effectively 3 ways to call select() for its nfds value: Using highest() + 1, using a manually specified n+1 value, and using None. Only the last case is tested and none of these are very well documented. The documentation for highest doesn't refer to select() at all and suggest that's where it might be used. Nor do the docs for select() refer to highest at all.

Susurrus · 2017-08-02T15:46:51Z

CHANGELOG.md

@@ -19,6 +19,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
  has changed type from `c_int` to `SockProtocol`.
  It accepts a `None` value for default protocol that was specified with zero using `c_int`.
  ([#647](https://github.com/nix-rust/nix/pull/647))
+- Made `select` easier to use, adding the ability to automatically calculate the `nfds` parameter using the new
+  `FdSet::largest` ([#701](https://github.com/nix-rust/nix/pull/701))


This should be "highest"

jonas-schievink · 2017-08-02T16:14:50Z

@Susurrus Fixed the changelog and updated the docs with links. I can also add another test if you tell me how it should look, but the current logic is so simple that I think a test is not warranted.

Susurrus · 2017-08-02T17:12:46Z

The point of the tests is as much about regressions and ergonomics testing as it is about logic. While the logic may be simple, there are 3 discrete ways to call select() and all 3 should have tests. This is important because it makes sure the API that we advertise to our users (3 different ways) always works since we have tests for all 3 ways. Please add the remaining tests.

Susurrus · 2017-08-03T15:26:52Z

src/sys/select.rs

+        fd_set.insert(r2);
+
+        let mut timeout = TimeVal::seconds(10);
+        assert_eq!(1, select(fd_set.highest(),


Isn't this not correct? Shouldn't it be fd_set.highest() + 1?

Oh, good catch!

It does concern me however that this test didn't fail when ran, however. Shouldn't it have?

Probably the fd that fired the select was the one with the lower number.

There any way to guarantee that's not the case so this test will fail if the +1 isn't there?

Well, that's why I opened the PR is the first place - the existing test wasn't threadsafe and could fail because of this. I guess you could put a mutex around every test that allocates fds...

asomers · 2017-08-11T15:26:50Z

@jonas-schievink I think we're all ready to commit this, but you'll need to rebase first to fix a trivial merge conflict in the CHANGELOG.

Doing this behind the scenes makes the API less error-prone and easier to use. It should also fix #679 (comment)

asomers · 2017-08-11T17:02:22Z

bors r+

701: Calculate `nfds` parameter for `select` r=asomers Doing this behind the scenes makes the API less error-prone and easier to use. It should also fix my issue in #679 (comment)

bors · 2017-08-11T20:27:55Z

Build succeeded

jonas-schievink commented Jul 20, 2017

View reviewed changes

Susurrus reviewed Jul 20, 2017

View reviewed changes

asomers self-requested a review July 20, 2017 23:07

jonas-schievink commented Jul 20, 2017

View reviewed changes

asomers requested changes Jul 21, 2017

View reviewed changes

Susurrus reviewed Jul 21, 2017

View reviewed changes

Susurrus suggested changes Jul 30, 2017

View reviewed changes

Susurrus suggested changes Jul 31, 2017

View reviewed changes

Susurrus approved these changes Aug 2, 2017

View reviewed changes

asomers approved these changes Aug 2, 2017

View reviewed changes

Susurrus suggested changes Aug 2, 2017

View reviewed changes

Susurrus reviewed Aug 3, 2017

View reviewed changes

Susurrus approved these changes Aug 3, 2017

View reviewed changes

Calculate nfds parameter for select

885a1f7

Doing this behind the scenes makes the API less error-prone and easier to use. It should also fix #679 (comment)

bors bot added a commit that referenced this pull request Aug 11, 2017

Merge #701

7324888

701: Calculate `nfds` parameter for `select` r=asomers Doing this behind the scenes makes the API less error-prone and easier to use. It should also fix my issue in #679 (comment)

bors bot merged commit 885a1f7 into nix-rust:master Aug 11, 2017

jonas-schievink deleted the better-select branch August 11, 2017 20:32

@@ @@ -31,6 +31,20 @@ fn test_fdset() { @@
                   }
               }
+              #[test]

Calculate nfds parameter for select #701

Calculate nfds parameter for select #701

Conversation

jonas-schievink commented Jul 20, 2017 • 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

Choose a reason for hiding this comment

jonas-schievink commented Jul 20, 2017

Choose a reason for hiding this comment

asomers 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

jonas-schievink commented Jul 21, 2017

jonas-schievink commented Jul 21, 2017

Choose a reason for hiding this comment

asomers commented Jul 30, 2017

jonas-schievink commented Jul 30, 2017

asomers commented Jul 31, 2017

Choose a reason for hiding this comment

jonas-schievink commented Aug 2, 2017

Susurrus commented Aug 2, 2017

asomers commented Aug 2, 2017

asomers commented Aug 2, 2017

asomers commented Aug 2, 2017

jonas-schievink commented Aug 2, 2017

Susurrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas-schievink commented Aug 2, 2017

Susurrus commented Aug 2, 2017

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

Susurrus Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asomers commented Aug 11, 2017

asomers commented Aug 11, 2017

bors bot commented Aug 11, 2017

Build succeeded

Calculate `nfds` parameter for `select` #701

Calculate `nfds` parameter for `select` #701

jonas-schievink commented Jul 20, 2017 •

edited

Loading

Susurrus Aug 4, 2017 •

edited

Loading