linux: Feature gate async runtime, allowing opting between Tokio or smol by jxs · Pull Request #27 · libp2p/if-watch

jxs · 2022-09-28T16:29:34Z

Following #21 and confirming @dignifiedquire's #21 (comment), we still can't define features per target.

But it seems we can define features for dependencies, and then dependencies can be defined per target.
With that we can apply the similar technique rtnetlink uses to support both runtimes, with the difference that in our case as smol_socket is the default (to maintain backwards compatibility) we invert the feature combination and if both are enabled tokio takes priority.

Only downside that comes to mind with this approach is if someone uses if-watch with default-features = false crate compilation breaks (which is also the reason this change is technically a breaking change). We can use compile_error! to give a better compilation error.

CC @thomaseizinger

dignifiedquire · 2022-09-28T20:21:49Z

nice solution 👍

thomaseizinger · 2022-09-29T00:29:32Z

+tokio = ["rtnetlink/tokio_socket"]
+smol_socket = ["rtnetlink/smol_socket"]


So what happens if I am on windows and activate the tokio feature flag?

So, I tried compiling with default features, tokio and --no-default-features.
All compile normally and produce the same Cargo.lock.
Btw should we name the features as rtnetlink names them or generify to smol and tokio ?

That is surprising to me. I would have expected cargo to choke on a feature that points to a dependency that is not present in the manifest for this target.

I'd prefer if we just name them after the executor and drop the _socket.

by making smol the default `IfWatcher` and introducing `TokioIfWatcher`.

thomaseizinger

Thanks! Couple of comments to ponder over :)

Is my understanding correct that rtnetlinks handling of sockets with this try_spawn_blocking stuff doesn't actually affect us because we are not using those APIs?

That is great news because I think it means we can actually fully encapsulate the way we enable executors without any upstream changes!

thomaseizinger · 2022-10-02T02:40:52Z

+tokio = ["rtnetlink/tokio_socket"]
+smol_socket = ["rtnetlink/smol_socket"]


I'd prefer if we just name them after the executor and drop the _socket.

thomaseizinger · 2022-10-02T02:49:18Z

 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

+## [2.1.0] - [unreleased]


Missing version bump in Cargo.toml and it is also breaking atm because you are removing the IfWatcher symbol for linux :)

mxinden

Thanks for looking into this @jxs!

mxinden · 2022-10-14T17:21:59Z

@jxs if I am not mistaken this is not blocked on anything, right? Not to rush, just to make sure you are not waiting on anyone's input.

thomaseizinger · 2022-10-18T07:47:05Z

    pub fn new() -> Result<Self> {
        let (tx, rx) = mpsc::channel(1);
-        std::thread::spawn(|| background_task(tx));
+        T::spawn(async { background_task(tx) });


Spawning a non-async function is a bit odd. Can we make it async?

I think this will block the executor thread otherwise.

I think this will block the executor thread otherwise.

interesting, why do you say this?

btw was able to test on a mac and confirm background_task's CFRunLoop::run_current() seems to run forever, and per tokio spawn_blocking doc for example:

This function is intended for non-async operations that eventually finish on their own. If you want to spawn an ordinary thread, you should use thread::spawn instead.

so we should probably maintain the plain old std::thread::spawn, wdyt?

I think this will block the executor thread otherwise.

interesting, why do you say this?

Unless interrupted otherwise, a task will run on an executor thread until it yields, i.e. returns Poll::Pending.

async_std has a mechanism to detect blocked executor threads and spawn new ones: https://async.rs/blog/stop-worrying-about-blocking-the-new-async-std-runtime/

Tokio doesn't though and it is a conscious design decision AFAIK.

btw was able to test on a mac and confirm background_task's CFRunLoop::run_current() seems to run forever, and per tokio spawn_blocking doc for example:

This function is intended for non-async operations that eventually finish on their own. If you want to spawn an ordinary thread, you should use thread::spawn instead.

But you are not using spawn_blocking right? So how is that relevant?

so we should probably maintain the plain old std::thread::spawn, wdyt?

Can we refactor this to be similar to the linux implementation where we register a waker and call it in a callback?

Unless interrupted otherwise, a task will run on an executor thread until it yields, i.e. returns Poll::Pending.

async_std has a mechanism to detect blocked executor threads and spawn new ones: https://async.rs/blog/stop-worrying->about-blocking-the-new-async-std-runtime/

Tokio doesn't though and it is a conscious design decision AFAIK.

Right yeah, but a function just by not being async doesn't mean it will block the thread.

But you are not using spawn_blocking right? So how is that relevant?

because background_task's CFRunLoop::run_current() seems to run forever, therefore blocking the thread.

Can we refactor this to be similar to the linux implementation where we register a waker and call it in a callback?

Yeah I think so, but there still still need for a blocking thread as CFRunLoop::run_current() is blocking, while the Windows NotifyIpInterfaceChange isn't.

Unless interrupted otherwise, a task will run on an executor thread until it yields, i.e. returns Poll::Pending.
async_std has a mechanism to detect blocked executor threads and spawn new ones: https://async.rs/blog/stop-worrying->about-blocking-the-new-async-std-runtime/
Tokio doesn't though and it is a conscious design decision AFAIK.

Right yeah, but a function just by not being async doesn't mean it will block the thread.

That is correct. I think we are saying the same thing, it is CFRunLoop::run_current() that is the problem.

But you are not using spawn_blocking right? So how is that relevant?

because background_task's CFRunLoop::run_current() seems to run forever, therefore blocking the thread.

Can we refactor this to be similar to the linux implementation where we register a waker and call it in a callback?

Yeah I think so, but there still still need for a blocking thread as CFRunLoop::run_current() is blocking, while the Windows NotifyIpInterfaceChange isn't.

That really is unfortunate. I guess we will have to stick with a regular thread then for MacOS.

That really is unfortunate. I guess we will have to stick with a regular thread then for MacOS.

yeah 😞 Reverted to an implementation similar to Windows ptal Thomas. Still, I think this approach makes sense and is more idiomatic the inicial one of overriding features

to fix android ci build. It was failing with: `libif_watch.so` doesn't exist.

thomaseizinger

Thanks, LGTM!

mxinden

Thanks for the thorough work on this.

mxinden · 2022-11-04T15:19:58Z

Tagged and published.

linux: Feature gate async runtime, allowing opting between Tokio or smol

824b0ad

thomaseizinger reviewed Sep 29, 2022

View reviewed changes

linux: make features trully addictive,

82e2cc9

by making smol the default `IfWatcher` and introducing `TokioIfWatcher`.

thomaseizinger reviewed Oct 2, 2022

View reviewed changes

mxinden reviewed Oct 2, 2022

View reviewed changes

Comment thread src/linux.rs

linux: encapsulate runtimes in their own modules

acf95c3

jxs force-pushed the feature-gate-runtime branch 2 times, most recently from 3a0c0ab to 7d6efc4 Compare October 17, 2022 19:47

general: feature gate every platform under tokio and smol

4818613

jxs force-pushed the feature-gate-runtime branch from 7d6efc4 to 4818613 Compare October 17, 2022 19:52

thomaseizinger reviewed Oct 18, 2022

View reviewed changes

jxs added 3 commits October 18, 2022 10:22

review: address suggestions

d597938

ci: fix android build step

bd399c2

fallback: remove unused Future import

ba8e1d4

jxs force-pushed the feature-gate-runtime branch from 2efad4b to ba8e1d4 Compare October 18, 2022 15:39

jxs added 2 commits October 20, 2022 16:56

ci: cdylib to crate-type on Cargo.toml,

66218b6

to fix android ci build. It was failing with: `libif_watch.so` doesn't exist.

apple: revert to previous implementation

9e7146e

thomaseizinger approved these changes Oct 22, 2022

View reviewed changes

rkuhn mentioned this pull request Oct 29, 2022

libp2p-0.49 with severe problems due to this crate (pulled in via mdns) #28

Closed

mxinden approved these changes Nov 4, 2022

View reviewed changes

Comment thread CHANGELOG.md Outdated

Update CHANGELOG.md

ba8cd7a

mxinden merged commit 31c4780 into libp2p:master Nov 4, 2022

		tokio = ["rtnetlink/tokio_socket"]
		smol_socket = ["rtnetlink/smol_socket"]

Uh oh!

Conversation

jxs commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dignifiedquire commented Sep 28, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomaseizinger left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mxinden commented Oct 14, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomaseizinger Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jxs Oct 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mxinden commented Nov 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jxs commented Sep 28, 2022 •

edited

Loading

thomaseizinger left a comment •

edited

Loading

thomaseizinger Oct 18, 2022 •

edited

Loading

jxs Oct 22, 2022 •

edited

Loading