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 example on wasm-unknown-unknown now working e2e #2868

Closed
wants to merge 3 commits into from
Closed

Conversation

rcoh
Copy link
Collaborator

@rcoh rcoh commented Jul 24, 2023

Motivation and Context

This PR will not be merged as is:

  1. A PR with exclusively the changes to aws-config
  2. A followup PR that creates a wiremock feature in aws-smithy-async
  3. A final PR to fixup the WASM integration test & add a wasm webpack test as well.

Description

Testing

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.

@github-actions
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

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

Awesome!

@@ -0,0 +1,7 @@
disallowed-methods = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should there be one of these in rust-runtime/ too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refactored to have a single file & symlinks

Comment on lines -150 to +152
start_time: self.start_time.unwrap_or_else(SystemTime::now),
start_time: self.start_time.unwrap_or_else(
#[allow(clippy::disallowed_methods)]
SystemTime::now,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we make these Option in the PresigningConfig and resolve to a default using TimeSource where they're used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I considered but I think these are actually used in customer code right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe they're only given in the builder when requesting a presigned URL.

@github-actions
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Split out a wiremock feature to allow test-util to compile without Hyper
@landonxjames
Copy link
Contributor

Pulled and built this locally to test and I am still seeing an error from LazyCredentialsCache when trying to run this in a WASM module locally.

Full error

RuntimeError: unreachable
    at __rust_start_panic (wasm://wasm/03173912:wasm-function[30766]:0x9204c9)
    at rust_panic (wasm://wasm/03173912:wasm-function[30315]:0x91f53b)
    at std::panicking::rust_panic_with_hook::h85b7f6628c291e12 (wasm://wasm/03173912:wasm-function[9274]:0x74133f)
    at std::panicking::begin_panic_handler::{{closure}}::h1e17bad04a5713a4 (wasm://wasm/03173912:wasm-function[11814]:0x7b41d9)
    at std::sys_common::backtrace::__rust_end_short_backtrace::h63adae5c31458c4b (wasm://wasm/03173912:wasm-function[30595]:0x920175)
    at rust_begin_unwind (wasm://wasm/03173912:wasm-function[19525]:0x88a7ec)
    at core::panicking::panic_fmt::hf5c4cd929d4aaa9e (wasm://wasm/03173912:wasm-function[25041]:0x8e9b3c)
    at std::time::SystemTime::now::h45f421f677a8c5a8 (wasm://wasm/03173912:wasm-function[23272]:0x8cfe1e)
    at <aws_smithy_async::time::SystemTimeSource as aws_smithy_async::time::TimeSource>::now::hd3f61c09a9ab34fb (wasm://wasm/03173912:wasm-function[17903]:0x868231)
    at aws_smithy_async::time::SharedTimeSource::now::h603e8ae0259e3b17 (wasm://wasm/03173912:wasm-function[14598]:0x813085)
    at <aws_credential_types::cache::lazy_caching::LazyCredentialsCache as aws_credential_types::cache::ProvideCachedCredentials>::provide_cached_credentials::h27fbf75720557611 (wasm://wasm/03173912:wasm-function[219]:0x217da6)
    at <aws_runtime::identity::credentials::CredentialsIdentityResolver as aws_smithy_runtime_api::client::identity::IdentityResolver>::resolve_identity::{{closure}}::hf9c369b6697af5a1 (wasm://wasm/03173912:wasm-function[825]:0x3a1bc3)
    at <core::pin::Pin<P> as core::future::future::Future>::poll::h267d42b0270901a5 (wasm://wasm/03173912:wasm-function[11963]:0x7ba083)
    at <aws_smithy_async::future::now_or_later::NowOrLater<T,F> as core::future::future::Future>::poll::hf7a8fe095f2c06c2 (wasm://wasm/03173912:wasm-function[4406]:0x5f83a0)
    at aws_smithy_runtime::client::orchestrator::auth::orchestrate_auth::{{closure}}::hfd2014f206334b51 (wasm://wasm/03173912:wasm-function[67]:0xa030a)
    at aws_smithy_runtime::client::orchestrator::try_attempt::{{closure}}::{{closure}}::hf0ee93372d9c1ce9 (wasm://wasm/03173912:wasm-function[58]:0x3a44d)
    at aws_smithy_runtime::client::orchestrator::try_attempt::{{closure}}::h83e973f3b360c463 (wasm://wasm/03173912:wasm-function[365]:0x2b3c3c)
    at aws_smithy_runtime::client::orchestrator::try_op::{{closure}}::{{closure}}::{{closure}}::hd3c270f970a7589c (wasm://wasm/03173912:wasm-function[281]:0x26300e)
    at <aws_smithy_runtime::client::timeout::MaybeTimeoutFuture<InnerFuture> as core::future::future::Future>::poll::h5a37214e910c83fe (wasm://wasm/03173912:wasm-function[2090]:0x4d9524)
    at aws_smithy_runtime::client::orchestrator::try_op::{{closure}}::{{closure}}::h089f0d3dd60c57e0 (wasm://wasm/03173912:wasm-function[56]:0x19193)
    at aws_smithy_runtime::client::orchestrator::try_op::{{closure}}::hb63d6ab2e20f2b7c (wasm://wasm/03173912:wasm-function[366]:0x2b4999)
    at aws_smithy_runtime::client::orchestrator::invoke_with_stop_point::{{closure}}::{{closure}}::{{closure}}::hc2908a8b8ec8ebe6 (wasm://wasm/03173912:wasm-function[1338]:0x43af1f)
    at <aws_smithy_runtime::client::timeout::MaybeTimeoutFuture<InnerFuture> as core::future::future::Future>::poll::hd37001a0c83b7809 (wasm://wasm/03173912:wasm-function[2021]:0x4ccf6f)
    at aws_smithy_runtime::client::orchestrator::invoke_with_stop_point::{{closure}}::{{closure}}::h76169f28cd2d0d28 (wasm://wasm/03173912:wasm-function[134]:0x183ff8)
    at <tracing::instrument::Instrumented<T> as core::future::future::Future>::poll::h573c541daa19c328 (wasm://wasm/03173912:wasm-function[1908]:0x4b7c95)
    at aws_smithy_runtime::client::orchestrator::invoke_with_stop_point::{{closure}}::hda25a78a700d2105 (wasm://wasm/03173912:wasm-function[273]:0x25a3ab)
    at aws_sdk_dynamodb::operation::list_tables::ListTables::orchestrate_with_stop_point::{{closure}}::hdaf23f5fa880a6ab (wasm://wasm/03173912:wasm-function[1383]:0x445d7c)
    at aws_sdk_dynamodb::operation::list_tables::ListTables::orchestrate::{{closure}}::h1435d6630406ab40 (wasm://wasm/03173912:wasm-function[581]:0x33ea94)
    at aws_sdk_dynamodb::operation::list_tables::builders::ListTablesFluentBuilder::send_orchestrator::{{closure}}::h4f236f2ee4b11049 (wasm://wasm/03173912:wasm-function[698]:0x3725df)
    at aws_sdk_dynamodb::operation::list_tables::builders::ListTablesFluentBuilder::send::{{closure}}::h4fa2b751a2f87755 (wasm://wasm/03173912:wasm-function[2645]:0x53101d)
    at wasmtest::list_tables::{{closure}}::h066c8247cf53116f (wasm://wasm/03173912:wasm-function[206]:0x2044db)
    at wasmtest::_::__wasm_bindgen_generated_list_tables::{{closure}}::h611a758e4a2fc3d7 (wasm://wasm/03173912:wasm-function[2326]:0x500d4e)
    at wasm_bindgen_futures::future_to_promise::{{closure}}::{{closure}}::hc7518afd82a92707 (wasm://wasm/03173912:wasm-function[1258]:0x42609d)
    at wasm_bindgen_futures::task::singlethread::Task::run::ha38635c516f7f77f (wasm://wasm/03173912:wasm-function[2787]:0x5449e6)
    at wasm_bindgen_futures::queue::QueueState::run_all::h78448b8373d4064c (wasm://wasm/03173912:wasm-function[2083]:0x4d8366)
    at wasm_bindgen_futures::queue::Queue::new::{{closure}}::hf74ff9b307427d82 (wasm://wasm/03173912:wasm-function[14716]:0x81677e)
    at <dyn core::ops::function::FnMut<(A,)>+Output = R as wasm_bindgen::closure::WasmClosure>::describe::invoke::h970e60bd77767285 (wasm://wasm/03173912:wasm-function[9365]:0x745d87)
    at __wbg_adapter_24 (/Volumes/workplace/wasmtest/pkg/wasmtest.js:243:10)
    at real (/Volumes/workplace/wasmtest/pkg/wasmtest.js:206:20)

I've uploaded a repro of my test, you will have to update Cargo.toml to point to your local version of the change and instructions for running are in the README. But at a high level, I am providing a time_source to the config (and the time source does work because I'm invoking and logging it successfully before the WASM file panics):

let shared_config = aws_config::from_env()
        .time_source(WasmTimeSource)
        .sleep_impl(WasmSleep)
        .region(Region::new("us-east-1"))
        .credentials_provider(credentials_provider)
        .credentials_cache(wasm_credentials_cache())
        .http_connector(DynConnector::new(Adapter::new(true)))
        .load()
        .await;

I am not providing a time source directly to the WASM compatible credentials cache (And the code panics if that bit is left out):

fn wasm_credentials_cache() -> CredentialsCache {
    let shared_sleep = SharedAsyncSleep::new(WasmSleep);
    CredentialsCache::lazy_builder()
        .sleep(shared_sleep)
        .into_credentials_cache()
}

But, the time_source method on the builder is marked #[doc(hidden)] so I don't think I should use it anyway.

I might just be doing this wrong, but if I'm not then this PR doesn't seem to fix the SystemTime::now issue for me.

My uneducated guess is that this bit in aws-config/src/lib.rs is not properly passing a time source to the credentials cache:

let credentials_cache = if credentials_provider.is_some() {
                Some(self.credentials_cache.unwrap_or_else(|| {
                    let mut builder =
                        CredentialsCache::lazy_builder().time_source(conf.time_source());
                    builder.set_sleep(conf.sleep());
                    builder.into_credentials_cache()
                }))
            }

The conf.time_source is only passed to the credentials_cache if the cache is not explicitly provided to the config. But you can't manually set a time_source on the provided credentials_cache because that field is marked #[doc(hidden)]

@rcoh
Copy link
Collaborator Author

rcoh commented Jul 31, 2023

these changse have all been merged in separate PRs

@rcoh rcoh closed this Jul 31, 2023
@jdisanti jdisanti deleted the wasm-fixes branch November 30, 2023 17:54
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