Add own abstraction layer over tokio::time::Interval #2318

sisou · 2024-03-16T16:48:46Z

Using gloo-timers for WASM. This change gets rid of the unmaintained wasm-timer crate, of which we were already using a fork with a fix from me anyway.

The resulting abstraction layer is very thin and since it exposes the exact same interface for both native and WASM targets, chunks of code that concerned themselves with the differentiation can be removed.

Pull request checklist

All tests pass. The project builds and runs.
I have resolved any merge conflicts.
I have resolved all clippy and rustfmt warnings.

sisou · 2024-03-17T20:21:04Z

I looked at the failing test it_can_limit_requests_rate: Before my force-push, which fixed most intervals to the previous behavior of having their first tick only after the first duration, it failed at the first limit reset already with a ConnectionClosed error. After, it only fails at the third limit reset. However, it's still not failing with limit-reset related problem, but because of a disconnecting peer.

I did not yet investigate the other failing test it_can_reset_requests_rate_with_reconnections.

sisou · 2024-04-11T16:50:58Z

@styppo you are ignoring two request-response tests in your commits here. Is there no way to enable them again? Or will you create a tracking issue for them?

hrxi

Found a small bug in test code. Plus, as @sisou mentions, tests seem to be ignored.

hrxi · 2024-04-29T10:03:17Z

handel/tests/mod.rs

@@ -300,7 +296,7 @@ async fn it_can_aggregate() {
    }

    loop {
-        match tokio::time::timeout_at(deadline, aggregation.next()).await {
+        match nimiq_time::timeout(timeout, aggregation.next()).await {


This changes the timeout logic from 300 ms * contributors / 7 for the entire thing to 300 ms * contributors / 7 per loop.

Do you mean the variable was copied wrong (deadline => timeout), or the new timeout method behaves differently than timeout_at did?

I think I mean neither.

Previously, the timeout_at function made sure that the entire loop did not take longer than timeout_ms. Now, the usage of the timeout function only makes sure that each loop iteration takes no longer than timeout.

I.e. previously the whole loop was limited in time, now each iteration can take as long as the whole loop previously.

Not sure if that made it clearer…

Yes, I understand now.

OK, so since we don't have a timeout_at anymore, we need to make the timeout duration dynamic. Calculate it from the difference of deadline - <now>. Would that be right?

Yes, that would work.

Done, check out a3fb606

hrxi · 2024-04-29T10:14:03Z

network-libp2p/src/network.rs

+            match result {
+                Err(_) => Err(RequestError::OutboundRequest(
+                    OutboundRequestError::SenderFutureDropped,
+                )),


Could you add a warn! here so the condition could be more easily noticed in logs?

hrxi · 2024-04-29T10:37:10Z

time/src/gloo.rs

+use gloo_timers::future::{IntervalStream, TimeoutFuture};
+use send_wrapper::SendWrapper;
+
+pub type Interval = SendWrapper<IntervalStream>;


From the send_wrapper docs:

This Rust library implements a wrapper type called SendWrapper which allows you to move around non-Send types between threads, as long as you access the contained value only from within the original thread.

This means we're locking our wasm implementation to one thread. I guess that's okay.

hrxi · 2024-04-29T10:41:42Z

time/src/tokio.rs

+    assert!(
+        period.as_millis() <= u32::MAX as u128,
+        "Period as millis must fit in u32"
+    );


A comment why this is done would be good. I assume it's due to the restriction on gloo's sleep function:

This function will panic if the specified Duration cannot be casted into a u32 in milliseconds.

hrxi · 2024-04-29T10:46:41Z

web-client/tests/wasm.rs

@@ -66,7 +66,9 @@ pub async fn it_can_initialize_with_mock_network() {
        }),
    );

-    spawn_local(consensus);


Why was this removed?

Because the NodeJS test runner was not exiting after the test completed successfully, because of pending timeouts or intervals. We tried to work around it by not spawning the consensus, but only polling it once (the added lines after this removed line), but that didn't work really either.

I have now changed the WASM tests to run in headless Chrome and there the test runner exits correctly, so I have added this back in.

- Add a utility function `spawn` which calls `wasm_bindgen_futures::spawn_local` for WASM targets and `tokio::spawn` otherwise. - Remove `TaskExecutor`.

…rs for WASM

* Remove most references to `tokio::time` * Remove `wasm_timer`

As the default NodeJS runtime fails to exit with pending timeouts or intervals, even after the tests all succeeded.

…tors

jsdanielh · 2024-05-09T16:31:49Z

handel/Cargo.toml

@@ -17,6 +17,7 @@ workspace = true
 async-trait = "0.1"
 futures = { package = "futures-util", version = "0.3", features = ["sink"] }
 log = { package = "tracing", version = "0.1", features = ["log"] }
+instant = { version = "0.1", features = [ "wasm-bindgen" ] }


I'd add the wasm-bindgen feature as a feature of this crate:

[features] wasm-bindgen = ["instant/wasm-bindgen"]

jsdanielh · 2024-05-09T16:34:27Z

network-libp2p/src/network.rs

+            // let timeout = Req::TIME_WINDOW.mul_f32(1.5f32);
+            // let future = response_rx.map_err(std::io::Error::other);
+            //match wasm_timer::TryFutureExt::timeout(future, timeout).await {


Suggested change

// let timeout = Req::TIME_WINDOW.mul_f32(1.5f32);

// let future = response_rx.map_err(std::io::Error::other);

//match wasm_timer::TryFutureExt::timeout(future, timeout).await {

jsdanielh · 2024-05-09T16:36:00Z

network-libp2p/Cargo.toml


 nimiq-bls = { workspace = true }
 nimiq-macros = { workspace = true }
 nimiq-network-interface = { workspace = true }
 nimiq-primitives = { workspace = true, features = ["policy"] }
 nimiq-hash = { workspace = true }
 nimiq-serde = { workspace = true }
+nimiq-time = { workspace = true }
 nimiq-utils = { workspace = true, features = [
    "tagged-signing",
    "libp2p",


Looks like we could remove the tokio-time feature.

jsdanielh · 2024-05-09T16:36:24Z

network-libp2p/tests/request_response.rs

@@ -605,6 +604,7 @@ async fn send_n_request_to_fail(net1: &Arc<Network>, net2: &Arc<Network>, n: u32

 #[cfg(feature = "tokio-time")]


Is this still needed?

jsdanielh · 2024-05-09T16:36:50Z

network-libp2p/tests/request_response.rs

@@ -727,6 +727,7 @@ async fn it_can_limit_requests_rate_after_reconnection() {

 #[cfg(feature = "tokio-time")]


Is this still needed?

jsdanielh · 2024-05-09T17:11:16Z

time/src/lib.rs

+
+#[cfg(target_family = "wasm")]
+pub use gloo::*;
+#[cfg(not(target_family = "wasm"))]


I'd do it differently by putting this into the Cargo.toml such that these dependencies get included only depending on the target. For this to work, the Cargo.toml should look like this:

[dependencies] [target.'cfg(target_arch = "wasm32")'.dependencies] gloo-timers = { version = "0.2", features = ["futures"], optional = true } [target.'cfg(not(target_arch = "wasm32"))'.dependencies] tokio = { version = "1.36", features = ["time"], optional = true } [target.'cfg(not(target_arch = "wasm32"))'.dependencies] tokio-stream = { version = "0.1", features = ["time"], optional = true }

Then the code here could just use the features tokio, tokio-stream and gloo for enabling code here.

jsdanielh · 2024-05-09T17:12:23Z

validator/Cargo.toml

@@ -21,6 +21,7 @@ workspace = true
 async-trait = "0.1"
 byteorder = "1.5"
 futures = { package = "futures-util", version = "0.3" }
+instant = { version = "0.1", features = [ "wasm-bindgen" ] }


I'd add a feature wasm-bindgen that selects this:

[features] wasm-bindgen = ["instant/wasm-bindgen"]

sisou added refactor Refactoring of some component WASM labels Mar 16, 2024

sisou requested review from styppo and hrxi March 16, 2024 16:48

sisou self-assigned this Mar 16, 2024

sisou force-pushed the sisou/nimiq-time branch from 634fb25 to 43b20e6 Compare March 17, 2024 20:16

styppo force-pushed the sisou/nimiq-time branch 3 times, most recently from ea2ef23 to c658661 Compare April 10, 2024 20:04

styppo approved these changes Apr 11, 2024

View reviewed changes

hrxi reviewed Apr 29, 2024

View reviewed changes

styppo and others added 7 commits May 7, 2024 23:25

WASM: add abstraction for tokio::spawn

91f3f52

- Add a utility function `spawn` which calls `wasm_bindgen_futures::spawn_local` for WASM targets and `tokio::spawn` otherwise. - Remove `TaskExecutor`.

Add own abstraction layer over tokio::time::Interval, using gloo-time…

b0697de

…rs for WASM

Network: ignore tests that rely on tokio::time::advance

a527449

WASM: Add abstractions for timeout and sleep

9ccd099

* Remove most references to `tokio::time` * Remove `wasm_timer`

Remove unused imports

6661f08

Consensus: remove Consensus poll timer

813fe33

Run all WASM tests in headless Chrome web-workers

09a9d76

As the default NodeJS runtime fails to exit with pending timeouts or intervals, even after the tests all succeeded.

sisou changed the base branch from albatross to styppo/spawn May 8, 2024 22:22

sisou force-pushed the sisou/nimiq-time branch from c658661 to 09a9d76 Compare May 8, 2024 22:22

Change Handel aggregate test back to use a deadline across all valida…

a3fb606

…tors

jsdanielh reviewed May 9, 2024

View reviewed changes

styppo force-pushed the styppo/spawn branch from 18a8aaa to db345de Compare May 10, 2024 14:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add own abstraction layer over tokio::time::Interval #2318

Add own abstraction layer over tokio::time::Interval #2318

sisou commented Mar 16, 2024 •

edited by styppo

sisou commented Mar 17, 2024 •

edited

sisou commented Apr 11, 2024

hrxi left a comment

hrxi Apr 29, 2024

sisou May 8, 2024

hrxi May 8, 2024

sisou May 8, 2024

hrxi May 9, 2024

sisou May 9, 2024

hrxi Apr 29, 2024

hrxi Apr 29, 2024

hrxi Apr 29, 2024

hrxi Apr 29, 2024

sisou May 8, 2024

jsdanielh May 9, 2024

jsdanielh May 9, 2024

jsdanielh May 9, 2024

jsdanielh May 9, 2024

jsdanielh May 9, 2024

jsdanielh May 9, 2024

jsdanielh May 9, 2024

	// let timeout = Req::TIME_WINDOW.mul_f32(1.5f32);
	// let future = response_rx.map_err(std::io::Error::other);
	//match wasm_timer::TryFutureExt::timeout(future, timeout).await {

		@@ -605,6 +604,7 @@ async fn send_n_request_to_fail(net1: &Arc<Network>, net2: &Arc<Network>, n: u32

		#[cfg(feature = "tokio-time")]

		@@ -727,6 +727,7 @@ async fn it_can_limit_requests_rate_after_reconnection() {

		#[cfg(feature = "tokio-time")]

Add own abstraction layer over tokio::time::Interval #2318

Are you sure you want to change the base?

Add own abstraction layer over tokio::time::Interval #2318

Conversation

sisou commented Mar 16, 2024 • edited by styppo

Pull request checklist

sisou commented Mar 17, 2024 • edited

sisou commented Apr 11, 2024

hrxi 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

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

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

Choose a reason for hiding this comment

sisou commented Mar 16, 2024 •

edited by styppo

sisou commented Mar 17, 2024 •

edited