Implement tokio support #2

jmagnuson · 2020-08-30T20:23:05Z

Adds dependencies tokio + futures behind 'tokio' feature.

Implements tokio::UnixSeqpacketConn and tokio::UnixSeqpacketListener for supporting SEQPACKET sockets in Tokio.

Closes #1.

Much of this roughly follows how tokio UnixStream/Listeners (SOCK_STREAM) are set up, but attempts to adopt some of the existing API naming used by uds. However, I'm totally open to other naming conventions, or module organization for that matter.

My immediate use-case is for UnixSeqPacketConn, but have also introduced (perhaps a bit more roughly) the Listener type for the sake of some sort of completeness.

jmagnuson · 2020-08-30T20:46:47Z

Cargo.toml

+
+[target."cfg(unix)".dev-dependencies]
+tempdir = "0.3"
+tokio_02 = {package="tokio", version = "0.2", features = ["macros", "rt-core", "test-util"]}


Ideally this would be optional as well, but I don't think Cargo allows optional dev-dependencies. Thus, it's either forcing these tokio test-related features for non-test builds, or forcing all test builds to take a tokio dependency.

I know 😞
This is by the way why the minimal-versions job is failing.
Either try to find the earliest patch version of tokio where cargo check -Z minimal-versions passes, or if the latest version doesn't pass either then remove the cargo test part of that job.

What's worse is that those feature flags added under dev-dependencies are also implicitly enabled for regular builds that enable that feature. For that reason i enable test-only mio_07 features through RUSTFLAGS in .cirrus.yml, .travis.yml and tests/test_{locally,target}.sh. While doing this might be pedantic since most final executables are going to want those features anyway, it could make those crates break when/if -Z features=dev_dep (which fixes that bug) becomes the default.

What do you think about moving the dev dependency features to RUSTFLAGS? If it's not pointless I can do that later.

Interesting; I wasn't aware of this issue, or workaround. I tried moving the dev features to RUSTFLAGS, but had some issues with macros and tokio's failure to pull in the tokio-macros sub-crate:

$ RUSTFLAGS='--cfg feature="os-poll" --cfg feature="rt-core" --cfg feature="macros"' cargo test --all-features ... error[E0432]: unresolved import `tokio_macros` --> /home/jon/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/tokio-0.2.22/src/lib.rs:403:13 | 403 | pub use tokio_macros::select_priv_declare_output_enum; | ^^^^^^^^^^^^ use of undeclared type or module `tokio_macros`

(note, I realized that test-util isn't actually used, so that will be dropped regardless of the outcome of this)

One workaround is just importing tokio-macros itself into dev-dependencies, and then setting up tests such that:

#[tokio_macros::test] async fn test_listener_accept() { ... }

which isn't something I've really seen other crates do (especially in documentation, which exclusively uses tokio::main), but I couldn't come up with a better solution offhand.

Thoughts?

There's also cargo-hack which looks to address these limitations in cargo. I did a quick trial run and it didn't seem to like running test with --all-features, but worked as expected when I wrote everything out manually:

$ cargo hack --features="mio_07,tokio,tokio_02/macros,tokio_02/rt-core,mio_07/os-poll" test

Thanks for the attempt. We'll just have to accept it being enabled for now.
(by the time I'm releasing v1.0 of this it will probably have been fixed in cargo :)

cargo-hack looks interesting but feels too complex.

jmagnuson · 2020-08-30T20:50:16Z

I'll add notes to the README and determine what needs to be done to satisfy OSX CI, but any interim reviews would be appreciated.

src/tokio/seqpacket.rs

tormol

Thanks!

You probably also need to update the MSRV to what tokio or futures require.
(The next uds release will be a major version due to other changes anyway).

Do you plan to add more methods? If not I'll add bind_unix_addr() and the like later.

tormol · 2020-08-31T20:52:51Z

Cargo.toml

+
+[target."cfg(unix)".dev-dependencies]
+tempdir = "0.3"
+tokio_02 = {package="tokio", version = "0.2", features = ["macros", "rt-core", "test-util"]}


I know 😞
This is by the way why the minimal-versions job is failing.
Either try to find the earliest patch version of tokio where cargo check -Z minimal-versions passes, or if the latest version doesn't pass either then remove the cargo test part of that job.

What's worse is that those feature flags added under dev-dependencies are also implicitly enabled for regular builds that enable that feature. For that reason i enable test-only mio_07 features through RUSTFLAGS in .cirrus.yml, .travis.yml and tests/test_{locally,target}.sh. While doing this might be pedantic since most final executables are going to want those features anyway, it could make those crates break when/if -Z features=dev_dep (which fixes that bug) becomes the default.

src/tokio/seqpacket.rs

jmagnuson · 2020-09-13T20:18:56Z

I went ahead and rebased the shutdown change (4d6c2c9) to go before everything else, resulting in a force-push. However, I left the existing changeset otherwise in-tact so that the subsequent fixup commits could be reviewed.

Do you plan to add more methods?

I think it might be best to leave that out for this initial pass. Not trying to push work onto you, but honestly you would probably get the tokio sockets up to parity a lot faster than I could. There's still the larger question of what to do about AsyncRead/Write traits that this PR should nail down first. And... making CI happy :(

tormol

Thanks for rebasing.
After changing the MSRV documentation, you can squash the later commits into one commit adding stream and another adding listener.

Sorry for being slow to respond. I was traveling last weekend and have been working late.

src/tokio/seqpacket.rs

tormol · 2020-09-16T22:22:43Z

Cargo.toml

+
+[target."cfg(unix)".dev-dependencies]
+tempdir = "0.3"
+tokio_02 = {package="tokio", version = "0.2", features = ["macros", "rt-core", "test-util"]}


What do you think about moving the dev dependency features to RUSTFLAGS? If it's not pointless I can do that later.

tormol · 2020-09-16T22:27:16Z

README.md

 ## Minimum Rust version

-The minimum Rust version is 1.36, because of `std::io::IoSlice`.
-If this is a problem I can make the parts that need it opt-out.
+The minimum Rust version is 1.39, because of the MSRV of both `tokio` and `futures`.


Since those dependencies are optional, this makes it unclear what the minimum supported version is when not enabling the tokio feature. Either don't mention the reason or add that the minimum version is 1.36 by default.

tormol · 2020-09-16T22:32:17Z

src/seqpacket.rs

@@ -650,6 +662,17 @@ impl NonblockingUnixSeqpacketConn {
        // nonblockingness is shared and therefore inherited
        Ok(NonblockingUnixSeqpacketConn { fd: cloned.into_raw_fd() })
    }
+
+    /// Shuts down the read, write, or both halves of this connection.
+    pub fn shutdown(&self, how: Shutdown) -> io::Result<()> {


Thanks for adding shutdown()!

Maybe add a test? (I can do that later though)

tormol · 2020-09-18T06:11:07Z

Forgot to say that the failing test is probably flaky or maybe apple changed something.

Adds a listener accept test that also demonstrates `tokio::UnixSeqpacketConn` usage.

jmagnuson · 2020-09-22T04:32:14Z

Went ahead and rebased again to clean things up since there are only one or two remaining items. But the latest changes were:

added shutdown tests for blocking, nonblocking, and tokio socket variants.
removed AsyncRead/Write impls
reworded MSRV to exclude mentioning tokio
removed test-util feature inclusion from tokio

Let me know your thoughts how what to do about RUSTFLAGS and tokio's macros feature, or if you want to punt and figure out what works best at a later time.

Sidenote: you may be right about CI flakiness. I see the latest commit c48d0af has passed, but I didn't make any explicit changes for that to happen..

jmagnuson · 2020-10-07T21:57:13Z

@tormol any further thoughts? Tokio is also on the verge of releasing 0.3 (which will invariably resemble 1.0 more closely than 0.2) so that is something to consider.

tormol · 2020-10-15T18:36:11Z

THANK YOU.

Sorry for being so slow to respond again.
(I lacked motivation to think about code outside of work for a while, but kept thinking I'd do it in a few days).
Now that I'm likely finished looking for an apartment to buy that will hopefully change.)

jmagnuson · 2020-10-15T22:49:38Z

No worries at all-- know that feeling all too well, and I actually just had my first child in September, so my OSS work has been pretty inconsistent as well. Glad to see the PR make it across the finish line though!

tormol · 2020-10-16T06:57:56Z

Congratulations! I'm hoping to test this on NetBSD and IllumOS and release this weekend. Before doing that I'm going to try to stabilize the macOS CI and commit some features that libc now supports. Den 16. oktober 2020 00:49:52 CEST, skrev Jon Magnuson <notifications@github.com>:

…

No worries at all-- know that feeling all too well, and I actually just had my first child in September, so my OSS work has been pretty inconsistent as well. Glad to see the PR make it across the finish line though! -- You are receiving this because you modified the open/close state. Reply to this email directly or view it on GitHub: #2 (comment)

tormol · 2020-10-21T20:31:57Z

0.2.0 is now released.

When I tested on Illumos, the test binaries created by cargo test --features tokio failed to start due to a missing symbol. I've just noted that in a feature table I've added to the README, but it could just be because I haven't updated that VM (OmniOS) in a while.

jmagnuson commented Aug 30, 2020

View reviewed changes

jmagnuson commented Aug 31, 2020

View reviewed changes

src/tokio/seqpacket.rs Outdated Show resolved Hide resolved

tormol requested changes Sep 6, 2020

View reviewed changes

jmagnuson force-pushed the feature/tokio branch from 4f96005 to 7445302 Compare September 13, 2020 17:54

tormol reviewed Sep 16, 2020

View reviewed changes

jmagnuson added 4 commits September 21, 2020 21:50

Implement shutdown() for UnixSeqpacketConn

ceb08b8

deps: add tokio + futures behind 'tokio' feature

e9ec9dd

Implement tokio::UnixSeqpacketConn

83cc148

Implement tokio::UnixSeqpacketListener

c48d0af

Adds a listener accept test that also demonstrates `tokio::UnixSeqpacketConn` usage.

jmagnuson force-pushed the feature/tokio branch from 67ccdbe to c48d0af Compare September 22, 2020 04:17

tormol approved these changes Oct 15, 2020

View reviewed changes

tormol merged commit 79f3336 into tormol:master Oct 15, 2020

jmagnuson deleted the feature/tokio branch October 15, 2020 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement tokio support #2

Implement tokio support #2

jmagnuson commented Aug 30, 2020

jmagnuson Aug 30, 2020

tormol Aug 31, 2020

tormol Sep 16, 2020

jmagnuson Sep 21, 2020

jmagnuson Sep 21, 2020

tormol Oct 15, 2020

jmagnuson commented Aug 30, 2020

tormol left a comment

tormol Aug 31, 2020

jmagnuson commented Sep 13, 2020

tormol left a comment

tormol Sep 16, 2020

tormol Sep 16, 2020

tormol Sep 16, 2020

tormol commented Sep 18, 2020

jmagnuson commented Sep 22, 2020

jmagnuson commented Oct 7, 2020

tormol commented Oct 15, 2020

jmagnuson commented Oct 15, 2020

tormol commented Oct 16, 2020 via email

tormol commented Oct 21, 2020

Implement tokio support #2

Implement tokio support #2

Conversation

jmagnuson commented Aug 30, 2020

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

jmagnuson commented Aug 30, 2020

tormol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmagnuson commented Sep 13, 2020

tormol 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

tormol commented Sep 18, 2020

jmagnuson commented Sep 22, 2020

jmagnuson commented Oct 7, 2020

tormol commented Oct 15, 2020

jmagnuson commented Oct 15, 2020

tormol commented Oct 16, 2020 via email

tormol commented Oct 21, 2020