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: time not implemented on wasm #2262

Closed

Conversation

eduardomourar
Copy link
Contributor

@eduardomourar eduardomourar commented Jan 30, 2023

Motivation and Context

Related to #2087 and awslabs/aws-sdk-rust#679 (reply in thread).

Whenever targeting a platform that the std::time::SystemTime::now() or std::time::Instant::now() is not supported, it will compile successfully but panic during runtime with the following message:

time not implemented on this platform

Description

In order to prevent that from happening, we will use the time library with the wasm-bindgen feature flag enabled. The crate aws-smithy-types will have that feature enabled by default and we will consume that function to get the current time for the system.

Testing

Unit tests already in place
Tested in the browser based on example here: awsdocs/aws-doc-sdk-examples#4325

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@rcoh
Copy link
Collaborator

rcoh commented Jan 30, 2023

I think we will probably want to figure out a way to do this without pulling in the wasm-bindgen — js-sys is a fairly meaty crate that I don't think we want to pull in for all users in all cases.

My guess is we'll want to refactor https://github.com/awslabs/smithy-rs/blob/main/aws/rust-runtime/aws-credential-types/src/time_source.rs into a dyn-safe trait like we have to AsyncSleep, pull it into aws-smithy-types, then make a WASM compatible implementation.

Probably worth an RFC to get everyone aligned on the design

@eduardomourar
Copy link
Contributor Author

eduardomourar commented Jan 30, 2023

I agree with you with pulling js-sys to all consumers is not a good idea (that was my poor implementation because we should be able to set that conditionally based on target). There is a big difference I see between AsyncSleep (as you can see it had to be reimplemented for the browser here), because this error can simply break your application during runtime silently and it can happen even when a dependency uses that function from std library.

I don't think we want to every time we need to fetch the current time to always go through the TimeSource (that was passed during instantiation or default), do you? I think this will somewhat become a burden while developing it. In the ideal scenario, we would want the standard library itself to not throw this error. This might happen when there is a standard way of dealing with of the host environment in a WebAssembly context (possibly WebAssembly component model will solve this).

@eduardomourar eduardomourar marked this pull request as draft January 30, 2023 15:22
@eduardomourar
Copy link
Contributor Author

eduardomourar commented Jan 30, 2023

@rcoh , I converted the PR back to draft. Let me know if you still think it is worth going on the route of this PR, otherwise I will park this for now.

@jdisanti
Copy link
Collaborator

jdisanti commented Jan 30, 2023

I had a discussion with @rcoh this morning about this. We want to take the following approach:

  • Add a TimeSource trait to aws-smithy-types
  • Make TimeSource configurable the exact same way as AsyncSleep/sleep_impl everywhere in smithy-rs
  • Refactor all places that consume time to use the TimeSource instead
  • Have a default TimeSource implementation that takes the time from Tokio's Instant to make testing easier since we will be able to use Tokio's time travel testing capabilities. This default would be wired up the same way as the default async sleep implementation.

You're welcome to implement this if you want, and I can provide more details and answer questions if you're interested.

On the fallibility of AsyncSleep, that browser use case you provided is interesting. Do you know why Delay is fallible in wasm-timer? From what I could tell from the source code, the timer can disappear, but when would that happen?

@eduardomourar
Copy link
Contributor Author

I had a discussion with @rcoh this morning about this. We want to take the following approach:

  • Add a TimeSource trait to aws-smithy-types
  • Make TimeSource configurable the exact same way as AsyncSleep/sleep_impl everywhere in smithy-rs
  • Refactor all places that consume time to use the TimeSource instead
  • Have a default TimeSource implementation that takes the time from Tokio's Instant to make testing easier since we will be able to use Tokio's time travel testing capabilities. This default would be wired up the same way as the default async sleep implementation.

You're welcome to implement this if you want, and I can provide more details and answer questions if you're interested.

Based on this comment from the wasm-timer maintainer, the approach you guys are proposing seems a good idea. Unfortunately, I will not be able to tackle this anytime soon.

@rcoh rcoh mentioned this pull request Apr 10, 2023
3 tasks
rcoh added a commit that referenced this pull request May 23, 2023
## Motivation and Context
- Controlling time is required for several testing use cases
- #2087 
- #2262 

## Description
Introduce `TimeSource` trait, a real implementation, and a test
implementation.

## Testing
These changes are used in the timestream PR

## Checklist
No changelog, these changes have no impact since the code is not yet
utilized

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
rcoh added a commit that referenced this pull request May 30, 2023
…hange warning is spurious) (#2728)

## Motivation and Context
- #2262 
- #2087 
- #2707 

This adds `TimeSource` in SDK and service configs so we can effectively
control time when executing requests with the SDK at the client level.

_note:_ the breaking change is in a trait implementation of a struct
we're going to delete, not a real breaking change

## Description
- Add `SharedTimeSource` and use it in SdkConfig / `aws-config` /
<service>::Config
- Wire up the signer and other uses of `SystemTime::now()` that I could
find
- track down broken tests

## Testing
- [x] various unit tests that all still pass

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
@rcoh
Copy link
Collaborator

rcoh commented May 31, 2023

implemented in #2728

@rcoh rcoh closed this May 31, 2023
@eduardomourar eduardomourar deleted the fix/time-now-wasm branch May 31, 2023 13:10
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.

3 participants