Supply web-time::SystemTime for wasm32

[DanGould]

This should be the last piece of logic required for rustls to support WebAssembly.

UnixTime::now() is called for certificate verification in rustls. std::time::SystemTime is unavailable in wasm32-unknown-unknown, so calling UnixTime::now() would panic before this change. web-time appears to be a sufficient std::time replacement for browser environments.

Regarding tests, I don't see any calls to UnixTime::now() in this crate, so It doesn't seem like testing against a wasm32 would prevent regressions that existing tests would. I'm still happy to set up wasm-bindgen-test if the maintainers would like to see those tests and run them and ci. Perhaps wasm32 tests make sense further up the stack in rustls where certificate verification happens and UnixTime::now() is called.

I did manually test a rustls connection from wasm32 using this change, however.

I plan to use this to make an https tunnel through a websocket connection in the browser

[djc]

Thanks! Can you add a CI job that targets a wasm32 target so that we know that this compiles and doesn't regress?

[DanGould]

Added wasm_build to build the lib with target wasm32-unknown-unknown in CI

I think this is what you were asking for, but also made the job run tests on another branch in case that was your preference .

[ctz]

Not sure about this.

First, it seems quite odd to have to poly-fill bits of std like this. It feels like you actually want to disable the std feature in this crate when you use it, and that should drop the availability of the panicking code and require the application to select a replacement. Alternatively, you could stick with std, but target a wasm target with a standard syscall interface like wasi (obv. not if you target browsers).

Second, it seems instant is not well maintained: sebcrozet/instant#52

[djc]

So maybe web-time is a good alternative? @daxpedda do you think it would be a good fit?

[daxpedda]

So maybe web-time is a good alternative? @daxpedda do you think it would be a good fit?

AFAICS the goal here is to find a working replacement for std::time::SystemTime on the Web platform. In that case web-time would be a good fit.

Keep in mind that wasm32-unknown-unknown doesn't necessarily mean Web, so if you want to support this target for non-Web platforms, you might want to guard this behind a feature. Obviously this would mean users who target wasm32-unknown-unknown without the feature will get a panic when they hit this part of the code.

[DanGould]

Thanks so much for the quick response. The most recent force-push should address the concerns raised here. It uses web-time, targets only wasm32-unknown-unknown and not WASI or Emscripten, and is guarded behind a new web feature.

[djc]

Obviously this would mean users who target wasm32-unknown-unknown without the feature will get a panic when they hit this part of the code.

Could/should we make this a compile time error?

[DanGould]

Cargo doesn't seem to support [target.'cfg(all(target_family = "wasm", target_os = "unknown"))'.features] syntax, so I removed that.

I've also feature gated UnixTime as suggested by @daxpedda. Those who target wasm32-unknown-unknown without the feature should no longer be able to refer to UnixTime and thus should not panic at runtime but get a compile time error instead.

I left the feature gates as-is so that wasm unknown targets running in std-supporting runtimes could use it. Is this something that happens, or do we want this to produce a compile error instead? If so, a feature gate seems suitable As explained by @daxpedda std and wasm unknown will always panic.

Calling SystemTime::now() will always panic when running it in wasm32-unknown-unknow. Unfortunately there is no way around this currently.

See the Rust Std source code on this.

target_family = "wasm", target_os = "unknown"	std feature	web feature	Call UnixTime::now()
✅	✅	✅	Uses web-time
✅	✅	❌	Compile error from feature gate
✅	❌	✅	Uses web-time
✅	❌	❌	Compile error
❌	✅	✅	Uses std::time
❌	✅	❌	Uses std::time
❌	❌	✅	Compile error from feature gate, web-time would default to std::time otherwise
❌	❌	❌	Compile error

[daxpedda]

I left the feature gates as-is so that wasm unknown targets running in std-supporting runtimes could use it. Is this something that happens, or do we want this to produce a compile error instead?

For more context: currently SystemTime::now() will always panic if you are running under wasm32-unknown-unknown. The only way this can change in the future is if a Wasm proposal is introduced that adds instructions to get time. However this is extremely unlikely to happen as this use-case is already covered by WASI, which has its own target.

[DanGould]

@daxpedda I took your advice and updated the table above. a wasm32-unknown-unknown target with std feature calling UnixTime::now() should now produce a compile error from the feature gate.

[cpu]

How will this interact with the continuation of the no-std work staged in the Rustls repo. Does the introduction of the TimeProvider in rustls/rustls#1502 and rustls/rustls#1688 reduce the need to address this within pki-types, or are the two orthogonal?

[daxpedda]

How will this interact with the continuation of the no-std work staged in the Rustls repo.

I'm not familiar with the no_std work in Rustls, but FWIW anything relying on wasm-bindgen won't be able to use no_std.

In context for this PR: wasm32-unknown-unknown can be no_std unless you enable the web crate feature.

[cpu]

I think the pull request description could use an update (it still refers to using instant as opposed to web-time).

Given the complexity of the matrix in this comment it also feels like there should be some attempt at documenting the new feature, its scope of support, and its limitations somewhere. Maybe in the top-level lib.rs rustdoc?

[DanGould]

Updated the PR description, applied your ci.yml suggestion, and made an attempt to document the web feature

[DanGould]

Are any further changes / approval required?

[cpu]

Within my limited knowledge of WASM and the wasm32-unknown-unknown target this seems reasonable. I flagged one docs nit and a YAML issue that could use quick fixes.

I also I think it'd be nice if the commit adding documentation was folded into the commit adding the feature, but we could probably do that for you before merge if it's a hassle to fix.

[DanGould]

adopted @cpu's recommendations and force pushed

[cpu]

Not sure about this.

@ctz Does the revised iteration based on web-time seem more reasonable to you or is this a non-starter from your perspective no matter the backing dep. filling in for system time on wasm32-unknown-unknown?

[DanGould]

Regarding @ctz's first concern

First, it seems quite odd to have to poly-fill bits of std like this. It feels like you actually want to disable the std feature in this crate when you use it, and that should drop the availability of the panicking code and require the application to select a replacement. Alternatively, you could stick with std, but target a wasm target with a standard syscall interface like wasi (obv. not if you target browsers).

This PR was opened so that I can target browsers, which this change alone would allow. It does seem possible to supply web-time by implementing TimeProvider further up the stack once rustls/rustls#1502 and rustls/rustls#1688 are merged, but the second is still a draft and I'm not sure on what timeline that would become available.

[cpu]

the second is still a draft and I'm not sure on what timeline that would become available.

I think it will be merged within a week or two ( 🤞 ). The main limiting factor is that I'll be out next week.

[DanGould]

@cpu rustls/rustls#1688 appears to be paused. Does the presence of that PR make this one obsolete? If that PR is paused and this one allows rustls to be embedded in browser environments, is it worth merging or does it create too much of a maintenance burden?

[djc]

If this PR means that web targets can use the default time provider, I think this is probably still a win?

[ctz]

I think I'm a bit less concerned now compared to before, given the rather platform-specific nature of this change.

But in the future I think we should resist further polyfills of broken std functionality.

[cpu]

Thanks for sticking with this and iterating based on our feedback @DanGould 👍

[DanGould]

Thanks for the feedback and your patience