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

Time side-effect in wasm32-unknown-unknown target #18

Closed
appcypher opened this issue Aug 25, 2022 · 8 comments · Fixed by #28
Closed

Time side-effect in wasm32-unknown-unknown target #18

appcypher opened this issue Aug 25, 2022 · 8 comments · Fixed by #28
Labels
bug Something isn't working

Comments

@appcypher
Copy link
Member

appcypher commented Aug 25, 2022

For non-wasi wasm32 build of rs-ucan, this line

SystemTime::now()

resolves to this branch of code that panics at runtime.

https://github.com/rust-lang/rust/blob/4d45b0745ab227feb9000bc15713ade4b99241ea/library/std/src/sys/unsupported/time.rs#L12-L14

Is it possible to have a version that has time passed from the interface down to where it is needed?

Any other solution?

@cdata
Copy link
Member

cdata commented Aug 25, 2022

@appcypher I would like to reproduce your issue before we discuss how to proceed. Can you clarify: by non-wasi do you mean wasm32-unknown-unknown or some other target?

@appcypher
Copy link
Member Author

appcypher commented Aug 25, 2022

@appcypher I would like to reproduce your issue before we discuss how to proceed. Can you clarify: by non-wasi do you mean wasm32-unknown-unknown or some other target?

Yes I mean wasm32-unknown-unknown. I plan to use the ucan crate from JavaScript; that may include wrapping and exposing interfaces like UcanBuilder, etc with wasm-bindgen. It is not a problem I have immediately but I see it being a problem when the time comes.

@cdata
Copy link
Member

cdata commented Aug 25, 2022

Please take a look at our CI: https://github.com/ucan-wg/rs-ucan/runs/6739585727?check_suite_focus=true#step:7:70

We run automated tests in Chrome to verify browser-targeted wasm32-unknown-unknown compatibility. Would you be willing to try running those tests locally and see if they pass for you?

Edit: I want to note that we may not be properly covering the code path you are hitting in our tests, so it's definitely still possible that there is a bug.

Also, @appcypher you should take a look at the work @fabricedesre is doing right now #17 - you two are probably doing the same thing at the same time ;D

@cdata
Copy link
Member

cdata commented Aug 25, 2022

Oh, and also while I have your attention @appcypher : would you be willing to add your use case as a comment on #19 ?

@cdata cdata added the bug Something isn't working label Aug 25, 2022
@appcypher
Copy link
Member Author

This code panics

#![cfg(test)]

use ucan::ucan::Ucan;
use wasm_bindgen_test::wasm_bindgen_test;

#[wasm_bindgen_test]
fn fail() {
    let token = "eyJhbGciOiJFZERTQSIsInR5cCI6IkpXVCIsInVjdiI6IjAuOC4xIn0.eyJpc3MiOiJkaWQ6a2V5Ono2TWtrODliQzNKclZxS2llNzFZRWNjNU0xU01WeHVDZ054NnpMWjhTWUpzeEFMaSIsImF1ZCI6ImRpZDprZXk6ejZNa2ZmRFpDa0NUV3JlZzg4NjhmRzFGR0ZvZ2NKajVYNlBZOTNwUGNXRG45Ym9iIiwiZXhwIjoxNjYxNDUwODk5LCJuYmYiOm51bGwsIm5uYyI6bnVsbCwiYXR0IjpbXSwiZmN0IjpbXSwicHJmIjpbXX0.liZl14IKOyc1eQluPJ39U4AoZLN_Ma9WKp9Vr1ak2VLFcglbZeIJmnEuuyWDRdo-81mv79cZ1tygaPwJrIKfCQ";
    let ucan = Ucan::try_from_token_string(token).unwrap();
    ucan.is_too_early(); // This panics because it calls `now()`.
}

Here is the repo for repro.

https://github.com/appcypher/rs-ucan-wasm-test

@fabricedesre
Copy link
Contributor

Weird, I have no problem with it in my wasm-api branch, testing with commit 50b614e

@appcypher
Copy link
Member Author

appcypher commented Aug 25, 2022

@fabricedesre That's because ucan crate, in a subtle way, assumes that all wasm32 targets are for the web.

ucan-key-support/web requires ucan/web, which requires instant/wasm-bindgen.

instant/wasm-bindgen thinks you want web APIs and branches to a web-based time implementation.

https://github.com/sebcrozet/instant/blob/master/src/wasm.rs#L183-L191

So it works in your case because you have ucan/web feature enabled. However, if I need to compile ucan to wasm32 that needs to be used in a non-web case, it fails.

@cdata
Copy link
Member

cdata commented Aug 25, 2022

@appcypher thank you for the test case. Can you share a few more details about how you plan to use the wasm32-unknown-unknown target (if not on the web)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants