Add own abstraction layer over tokio::time::Interval #2318
Conversation
|
I looked at the failing test I did not yet investigate the other failing test |
styppo
force-pushed
the
sisou/nimiq-time
branch
3 times, most recently
from
ea2ef23
to
c658661
Compare
|
@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? |
| use gloo_timers::future::{IntervalStream, TimeoutFuture}; | ||
| use send_wrapper::SendWrapper; | ||
|
|
||
| pub type Interval = SendWrapper<IntervalStream>; |
There was a problem hiding this comment.
From the send_wrapper docs:
This Rust library implements a wrapper type called
SendWrapperwhich allows you to move around non-Sendtypes 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.
| @@ -66,7 +66,9 @@ pub async fn it_can_initialize_with_mock_network() { | |||
| }), | |||
| ); | |||
|
|
|||
| spawn_local(consensus); | |||
There was a problem hiding this comment.
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.
handel/Cargo.toml
Outdated
| @@ -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" ] } | |||
There was a problem hiding this comment.
I'd add the wasm-bindgen feature as a feature of this crate:
[features]
wasm-bindgen = ["instant/wasm-bindgen"]
|
|
||
| #[cfg(target_family = "wasm")] | ||
| pub use gloo::*; | ||
| #[cfg(not(target_family = "wasm"))] |
There was a problem hiding this comment.
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.
validator/Cargo.toml
Outdated
| @@ -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" ] } | |||
There was a problem hiding this comment.
I'd add a feature wasm-bindgen that selects this:
[features]
wasm-bindgen = ["instant/wasm-bindgen"]
e6f05dd
to
7c4ad3d
Compare
* Remove most references to `tokio::time` * Remove `wasm_timer`
Using
gloo-timersfor WASM. This change gets rid of the unmaintainedwasm-timercrate, 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
clippyandrustfmtwarnings.