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

fix: Replace std::time::Instant by wasm_timer::Instant #2135

Closed
wants to merge 1 commit into from

Conversation

fusetim
Copy link

@fusetim fusetim commented Jul 11, 2021

Closes #2134
Needs tomaka/wasm-timer#17

  • Just replaced the imports of std::time::Instant with wasm-timer::Instant for WASM support.
  • Checked the existing uses of Instant, there is no dependency outside of the crate itself. All uses were never publicly exposed.
  • I can't run the tests on my PC, it became really unstable when I tried but it fully compiled (even on wasm32 target using the Add missing methods to Instant, matching Rust 1.39 tomaka/wasm-timer#17).

* I checked the existing uses of Instant, there is no dependency
outside the crate itself.

* I can't run the test on my PC, it became really unstable when I tried
but it fully compiled.
@dvc94ch
Copy link
Contributor

dvc94ch commented Jul 12, 2021

mdns won't work in a browser as it requires access to a udp socket. can you tell us more about what platform you're trying to run on?

@fusetim
Copy link
Author

fusetim commented Jul 12, 2021

Yes you are right, in the case of mDNS it doesn't change anything, nevertheless we can think that some people could see the utility of using it with Node.js and in this case, Node.js has the capacity to open UDP sockets.

But in my case, it's about creating a custom node with a future protocol that I have to create, based on rust-libp2p and to be compiled with the target wasm32-unknown-unknown. It should run in the browser with some wasm-bindgen stuff.

I encountered an error with Gossipsub (when verifying messages) because it was using std::time::Instant::now() which is not implemented on the wasm32-unknown-unknown target today. In fact, I quickly went around to fix these imports and now it works perfectly following the changes made by this PR.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, using wasm_timer across the project sounds good to me.

Yes you are right, in the case of mDNS it doesn't change anything, nevertheless we can think that some people could see the utility of using it with Node.js and in this case, Node.js has the capacity to open UDP sockets.

I would expect there to be additional blockers when using libp2p-mdns on Node.js. That said, I don't think it hurts to use wasm_timer in libp2p-mdns as well.

I left a comment on your wasm_timer pull request. Once that pull request is merged and released we can continue here.

Thanks for creating the patch @fusetim.

@dvc94ch
Copy link
Contributor

dvc94ch commented Dec 16, 2021

believe this one was fixed by @wngr

@thomaseizinger
Copy link
Contributor

believe this one was fixed by @wngr

Yep, thanks for the ping. Closing this.

@fusetim
Copy link
Author

fusetim commented Dec 18, 2021

For tracking, fixed by #2245. Thanks.

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 this pull request may close these issues.

WASM regression: Usage of std::time::Instant
4 participants