Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WASM regression: Usage of std::time::Instant #2134

Closed
4 tasks
fusetim opened this issue Jul 11, 2021 · 4 comments
Closed
4 tasks

WASM regression: Usage of std::time::Instant #2134

fusetim opened this issue Jul 11, 2021 · 4 comments

Comments

@fusetim
Copy link

fusetim commented Jul 11, 2021

While creating my custom node based on rust-libp2p, I rediscovered some uses of std::time::Instant in the Gossipsub and Kadmelia modules. As previously done by the PR #1071 (issue #900), the remaining std::time::Instant should again be replaced with wasm_timer::Instant to allow Wasm compatibility.

The spotted ones:

@fusetim
Copy link
Author

fusetim commented Jul 11, 2021

We will need an update of wasm-timer, I think, but otherwise nothing seems to break and in every case, the use of Instant was inside the crate only.

Compiling libp2p-gossipsub v0.32.0 (/mnt/storagedisk/Developing/IntelliJ/rust-libp2p/protocols/gossipsub)
error[E0599]: no method named `checked_add` found for struct `wasm_timer::Instant` in the current scope
   --> /mnt/storagedisk/Developing/IntelliJ/rust-libp2p/protocols/gossipsub/src/peer_score.rs:848:34
    |
848 | ...                   .checked_add(topic_params.mesh_message_deliveries_window)
    |                        ^^^^^^^^^^^ method not found in `wasm_timer::Instant`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0599`.
error: could not compile `libp2p-gossipsub

EDIT: And here is the changes needed: tomaka/wasm-timer#17

@fusetim
Copy link
Author

fusetim commented Dec 18, 2021

Fixed by #2245.

However, I am still worried about this import in Kadmelia in "master" (f701b24 at the time).

use std::time::{Duration, Instant};

let elapsed = Instant::now() - Duration::from_secs(1);

I might be missing something, but it seems to me, that KAD should be WASM-compatible, and the other inner modules seems to use a correct instant::Intant import. May @wngr could enlighten this ?

@wngr
Copy link
Contributor

wngr commented Dec 18, 2021

From what I see this is only used in a test, that's why I didn't change it. Should have changed the import scope, though.

@fusetim
Copy link
Author

fusetim commented Dec 18, 2021

Oh you're right, I missed the #[test], the issue can be closed now. Thank you for your time 👍

@fusetim fusetim closed this as completed Dec 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants